vkd3d-shader: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Merge request reports
Activity
I haven't quite made it through all the changes in patch 4/4, but some initial comments:
From patch 1/4:
+void vkd3d_shader_sm6_free_shader_signature(struct vkd3d_shader_sm6_signature *signature) +{ + vkd3d_free(signature->elements); + signature->elements = NULL; +}
If this were part of the public API, this should be called "vkd3d_shader_free_sm6_signature()"; for an internal API, the convention would be "vkd3d_shader_sm6_signature_cleanup()". We (Zeb, mostly) have also been omitting the vkd3d_/vkd3d_shader_ prefix on new (module) internal API. I'm not too bothered by either variant, but if you prefer, you could also go with "struct sm6_signature" and "sm6_signature_cleanup()".
+static bool shader_signature_from_shader_sm6_signature(struct vkd3d_shader_signature *signature, + const struct vkd3d_shader_sm6_signature *src) +{ [...] + if (!(signature->elements = vkd3d_malloc(signature->element_count * sizeof(*signature->elements)))) + return false;
This works, but only because sizeof(*signature->elements) is smaller than sizeof(*src->elements). That may be more fragile than we care for; we'd normally use vkd3d_calloc() here, like shader_parse_signature() does.
From patch 2/4:
+ static const DWORD vs_code[] = + { +#if 0 + uint3 index; + + struct vs_data + { + float4 pos : SV_Position; + float4 color[3] : COLOR; + }; + + void main(in struct vs_data vs_input, out struct vs_data vs_output) + { + vs_output.pos = vs_input.pos; + vs_output.color[0] = vs_input.color[index.x]; + vs_output.color[1] = vs_input.color[index.y]; + vs_output.color[2] = vs_input.color[index.z]; + } +#endif [...] + }; + static const D3D12_SHADER_BYTECODE vs = {vs_code, sizeof(vs_code)}; + static const DWORD ps_code[] = + { +#if 0 + uint4 index; + + struct ps_data + { + float4 pos : SV_Position; + float4 color[3] : COLOR; + }; + + float4 main(struct ps_data ps_input) : SV_Target + { + return ps_input.color[index.w]; + } +#endif [..] + };
Perhaps the answer is simply "no", but is there any chance the HLSL compiler is already (or soon...) able to compile these and we could use something like hlsl_d3d12.c's "compile_shader" for these?
From patch 3/4:
+ /* The normaliser requires the highest slot free in some cases. */ + switch (param->type) + { + case VKD3DSPR_INPUT: + case VKD3DSPR_OUTPUT: + case VKD3DSPR_COLOROUT: + case VKD3DSPR_INCONTROLPOINT: + case VKD3DSPR_OUTCONTROLPOINT: + case VKD3DSPR_PATCHCONST: + vkd3d_shader_parser_error(&priv->p, VKD3D_SHADER_ERROR_TPF_PARAM_ORDER_TOO_HIGH, + "Unexpected I/O parameter order greater than 2.\n"); + break; + }
It's not introduced by this patch, but unfortunately I only notice this issue now: It seems that during the vkd3d_shader_instruction_array conversion a number of checks for "parser->failed" ended up getting dropped. In a lot of cases we also set VKD3DSIH_INVALID, and some of the remaining checks end up catching that and mitigating the issue, but we should be checking for that flag in vkd3d_shader_sm4_parser_create() and vkd3d_shader_sm1_parser_create().
Both the comment and the error message are perhaps slightly inaccurate; from the point of view of the parser, the issue here is that (depending on the shader type and potentially the phase) shader registers have a particular dimension. Addressing them using either more or fewer indices would be invalid. Perhaps we don't immediately need to be quite that strict (although I suppose we could extend register_type_table[] to include the required information), but I think the error should essentially be something along the lines of "invalid index count for register type".
From patch 4/4:
+static void VKD3D_PRINTF_FUNC(3, 4) shader_normaliser_error(struct vkd3d_shader_normaliser *normaliser, + enum vkd3d_shader_error error, const char *format, ...) +{ + struct vkd3d_shader_location location = {NULL, normaliser->source_index + 1, 0}; + va_list args; + + va_start(args, format); + vkd3d_shader_verror(normaliser->message_context, &location, error, format, args); + va_end(args); + normaliser->failed = true; +}
That drops the original "source_name" field. We should be able to store that in the vkd3d_shader_instruction_array structure, but we could also just store a vkd3d_shader_location structure inside the vkd3d_shader_instruction structure, which the normaliser could then point to. This can all be tested with vkd3d-compiler and an appropriate shader, of course.
+static unsigned int shader_signature_find_element_for_reg(const struct vkd3d_shader_sm6_signature *signature, + unsigned int reg_idx, unsigned int write_mask, struct vkd3d_shader_normaliser *normaliser) +{ + unsigned int signature_idx; + + for (signature_idx = 0; signature_idx < signature->element_count; ++signature_idx) + { [...] + } + + shader_normaliser_error(normaliser, VKD3D_SHADER_ERROR_IR_INVALID_IO_REGISTER, + "Could not find signature element matching register %u and write mask %#x.\n", reg_idx, write_mask); + return ~0u; +}
Technically we're iterating over signature elements above, not signatures; that makes "signature_idx" perhaps a bit awkward as a variable name. We could also just use something like "i", of course.
I think we could return a pointer to the element itself here, instead of the index. (Like vkd3d_shader_find_signature_element(), incidentally.) All the current callers seem to just use the index to get at the element.
Somewhat similarly, it probably makes sense to call shader_normaliser_error() from the callers instead of here. On the one hand, all the callers already do some error handling anyway, while on the other hand I could imagine callers potentially being able to give better error messages than we can here.
+static int signature_element_mask_compare(const void *a, const void *b) +{ + const struct vkd3d_shader_sm6_signature_element *e = a, *f = b; + + if (e->mask == f->mask) + return vkd3d_u32_compare(e->register_index, f->register_index); + return vkd3d_u32_compare(e->mask, f->mask); +}
The code above works, but it is a bit unusual. The usual way to write that would be:
if ((ret = vkd3d_u32_compare(e->mask, f->mask))) return ret; return vkd3d_u32_compare(e->register_index, f->register_index);
+static bool merge_sysval_semantics(struct vkd3d_shader_sm6_signature_element *e, + struct vkd3d_shader_sm6_signature_element *f) +{ [...] +}
Despite the name, this function does not merge anything. Also, the parameters could be const.
Unfortunately I see no reasonable way to split the last patch. It's all interdependent.
Introducing "source_index" seem reasonably separate, at least.
This works, but only because sizeof(*signature->elements) is smaller than sizeof(*src->elements).
I changed it to
vkd3d_calloc()
, but I see no size mismatch here.I think we could return a pointer to the element itself here, instead of the index.
The index is used to address
dcl_params
, and it's stored in a register as an index.
We (Zeb, mostly) have also been omitting the vkd3d_/vkd3d_shader_ prefix on new (module) internal API. I'm not too bothered by either variant, but if you prefer, you could also go with "struct sm6_signature" and "sm6_signature_cleanup()".
My reasoning is that firstly, I like a clear distinction between what is and isn't external API, and secondly, typing out vkd3d_shader everywhere makes names annoyingly long.
I've said this a few times, but along the same lines, I'd like to propose giving struct vkd3d_shader_instruction (and related structures / functions) a different, shorter prefix in place of "vkd3d_shader". Some ideas:
- "ir" (to match the file name. Granted, this isn't the only IR in vkd3d-shader, but it's also probably the only one that will have a generic role in every transformation),
- "vsir" (an abbreviation of "vkd3d-shader IR"),
- "vazir" (a more cute/clever name, Persian for "minister" but which I understand can also mean something close to "helper" or "assistant", which seems quite fitting. I've tried to also make it into an acronym but the 'z' makes things difficult).
Perhaps the answer is simply "no", but is there any chance the HLSL compiler is already (or soon...) able to compile these and we could use something like hlsl_d3d12.c's "compile_shader" for these?
We need to handle array semantics (148, which I've signed off on and is hopefully just waiting for Gio's sign-off) and then relative addressing (which isn't done but looks pretty easy). I'm of the opinion it shouldn't block this patch, though.
FWIW, given the simplicity of the test it should even be possible to port it to a shader_test file :-)
added 11 commits
-
02c19a64...af4bb037 - 7 commits from branch
wine:master
- 988bcceb - vkd3d-shader: Introduce an internal sm6 signature structure.
- 51ee0594 - tests/d3d12: Test register relative addressing in vertex and pixel shaders.
- 63b52236 - vkd3d-shader/tpf: Fail parsing if an input/output parameter order is > 2.
- 8fc936e1 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit list-
02c19a64...af4bb037 - 7 commits from branch
added 6 commits
- 8e42b7c3 - vkd3d-shader/d3dbc: Return an error from vkd3d_shader_sm1_parser_create() if the parser failed.
- 7d4c2008 - vkd3d-shader/tpf: Return an error from vkd3d_shader_sm4_parser_create() if the parser failed.
- 35c119cd - vkd3d-shader: Introduce an internal sm6 signature structure.
- 8b25e7c5 - tests/d3d12: Test register relative addressing in vertex and pixel shaders.
- 84a6cea5 - vkd3d-shader/tpf: Fail parsing if an input/output parameter order is > 2.
- f64eb9d0 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit listadded 1 commit
- 1dcd92fc - vkd3d-shader: Add source information to normaliser errors and warnings.
This works, but only because sizeof(*signature->elements) is smaller than sizeof(*src->elements).
I changed it to
vkd3d_calloc()
, but I see no size mismatch here.struct sm6_signature_element has the extra "register_count" field that struct vkd3d_shader_signature_element doesn't have; that was the point of this patch, of course. So if "element_count * sizeof(struct sm6_signature_element)" didn't overflow, "element_count * sizeof(struct vkd3d_shader_signature_element) wouldn't either. Though I gather from your reply that that was not the reasoning behind using vkd3d_malloc(). :)
I think the first 5 patches in the series are fine, and could go in as a separate series.
Patch 7/7 doesn't touch spirv.c and glsl.c, and I think it should. I.e., spirv_compiler_generate_spirv() and vkd3d_glsl_generator_generate() would ideally use the instruction's "source_index" to set the location. Those parts could do go in independently of the changes in patch 6/7.
Some comments on patch 6/7:
-
Most (all?) of the VKD3D_SHADER_ERROR_IR_* errors seem like things that the frontend could/should catch. Doing them in the frontend would also allow introducing that validation as separate patches. We could possibly introduce a separate validation pass for that in tpf.c, but I don't think we strictly have to.
-
There's a fair amount of shared state introduced to struct vkd3d_shader_normaliser, but most of it seems local to a particular transformation pass. It probably doesn't need to be part of the vkd3d_shader_normaliser structure, and it probably doesn't even need to be visible outside of ir.c. To some extent that's true for some of the existing shared state in struct vkd3d_shader_normaliser as well.
-
I think we should add the different signatures to struct vkd3d_shader_instruction_array instead of struct vkd3d_shader_normaliser. It would probably simplify passing around the signatures in a couple of other places as well. (In particular, places that currently either use struct vkd3d_shader_desc or pass signatures around as separate parameters.)
-
I don't think the use of the ternary operator in signature_element_mask_compare() improves readability.
-
Somewhat similarly, the "if ((array_size = (input->register_count > 1) ? input->register_count : 0))" line in spirv_compiler_emit_default_control_point_phase() seems a bit too clever. I'd suggest doing something like the following:
if ((array_size = input->register_count) > 1) type_id = vkd3d_spirv_get_op_type_array(...); else array_size = 0;
-
The patch introduces "outer_array_length" parameters to spirv_compiler_emit_array_variable() and spirv_compiler_emit_builtin_variable(), but the conventional way we'd do that would be to make "array_length"/"array_size" and array and add something like an "index_count"/"length_count"/"size_count" parameter. For example, like we do with vkd3d_spirv_build_op_access_chain() and vkd3d_spirv_build_op_access_chain1(). These changes could be split out as a separate patch. (E.g. "Support emitting multi-dimensional array variables in spirv_compiler_emit_array_variable().")
-
I'm tempted to suggest adding an "index_count" field to struct vkd3d_shader_register. (As opposed to comparing the "offset" field of register indices with ~0u.)
-
mentioned in merge request !197 (merged)
added 56 commits
-
3474d726...8b57a612 - 48 commits from branch
wine:master
- 655cd577 - vkd3d-shader/tpf: Validate input/output register index counts.
- 6ce8687c - vkd3d-shader/tpf: Validate signature element register indices.
- 94ca6552 - vkd3d-shader/tpf: Validate signature element masks.
- 469de967 - vkd3d-shader/tpf: Validate input/output registers.
- 0787554e - vkd3d-shader/tpf: Validate index range declarations.
- 8b4d359f - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
- f043e656 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.
- 8c9bd96c - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit list-
3474d726...8b57a612 - 48 commits from branch
added 10 commits
- f03cd243 - vkd3d-shader/tpf: Validate input/output register index counts.
- 542b3d77 - vkd3d-shader/tpf: Validate signature element register indices.
- 7d5357e8 - vkd3d-shader/tpf: Validate signature element masks.
- 98bec075 - vkd3d-shader/tpf: Validate input/output registers.
- e6b3ca0b - vkd3d-shader/tpf: Validate index range declarations.
- aea7fe8f - vkd3d-shader/tpf: Remove an unnecessary carriage return from a parser error message.
- 8f9cbe03 - vkd3d-shader/tpf: Validate input and output index ranges for default control point phases.
- 97496f8b - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
- 85914131 - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
- 6dabb0b8 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit listadded 40 commits
-
6dabb0b8...dc414449 - 37 commits from branch
wine:master
- 0b1e5997 - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
- 39f2bd4f - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
- d8db226b - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit list-
6dabb0b8...dc414449 - 37 commits from branch
added 1 commit
- 5200a79c - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
added 1 commit
- 3cabc5b3 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
4602 4620 { 4603 4621 if (builtin) 4604 4622 { 4605 input_id = spirv_compiler_emit_builtin_variable(compiler, builtin, storage_class, array_size); 4623 input_id = spirv_compiler_emit_builtin_variable_v(compiler, builtin, storage_class, &array_size, 1); 923 signature = normaliser->output_signature; 924 break; 925 default: 926 return; 927 } 928 929 id_idx = reg->idx_count - 1; 930 reg_idx = reg->idx[id_idx].offset; 931 write_mask = VKD3DSP_WRITEMASK_0 << vkd3d_swizzle_get_component(src_param->swizzle, 0); 932 element_idx = shader_signature_find_element_for_reg(signature, reg_idx, write_mask); 933 934 e = &signature->elements[element_idx]; 935 if ((e->register_count > 1 || sysval_semantic_is_tess_factor(e->sysval_semantic))) 936 id_idx = shader_register_normalise_arrayed_addressing(reg, id_idx, e->register_index); 937 reg->idx[id_idx].offset = element_idx; 938 reg->idx_count = id_idx + 1; - Comment on lines +937 to +938
978 keep = shader_dst_param_io_normalise(&ins->declaration.register_semantic.reg, true, 979 normaliser); 980 break; 981 case VKD3DSIH_HS_CONTROL_POINT_PHASE: 982 case VKD3DSIH_HS_FORK_PHASE: 983 case VKD3DSIH_HS_JOIN_PHASE: 984 normaliser->phase = ins->handler_idx; 985 memset(normaliser->input_dcl_params, 0, sizeof(normaliser->input_dcl_params)); 986 memset(normaliser->output_dcl_params, 0, sizeof(normaliser->output_dcl_params)); 987 memset(normaliser->pc_dcl_params, 0, sizeof(normaliser->pc_dcl_params)); 988 break; 989 default: 990 if (shader_instruction_is_dcl(ins)) 991 break; 992 for (i = 0; i < ins->dst_count; ++i) 993 shader_dst_param_io_normalise((struct vkd3d_shader_dst_param *)&ins->dst[i], false, normaliser); One side effect of this MR is that the indices in
struct vkd3d_shader_register
begin having a different meaning depending on whether they have to be interpreted as SM4-like or SM6-like, which is dictated by the context (right now the context amounts to begin before or afterinstruction_array_normalise_io_registers()
, but in the future it might become more complicated). I think this can be handled, but I don't find ideal. Maybe there should be a flag at some point of thevkd3d_shader_instruction
structure tree that should keep track of that information? I don't know, just thinking out loud.added 21 commits
-
38c9db09...855bb71f - 18 commits from branch
wine:master
- 756658d8 - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
- 4a7e7462 - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
- 29c5e4d5 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit list-
38c9db09...855bb71f - 18 commits from branch
added 5 commits
-
171747ce...e060773c - 2 commits from branch
wine:master
- 31682c52 - vkd3d-shader/spirv: Support emitting multi-dimensional array variables.
- 110e48e5 - vkd3d-shader/ir: Eliminate struct vkd3d_shader_normaliser.
- 6835e817 - vkd3d-shader/ir: Normalise signatures and input/output registers to the Shader Model 6 pattern.
Toggle commit list-
171747ce...e060773c - 2 commits from branch