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:
```c 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.