This seems generally fine to me. Couple of points:
```diff -void shader_instruction_init(struct vkd3d_shader_instruction *ins, enum vkd3d_shader_opcode handler_idx) +void shader_instruction_init(struct vkd3d_shader_instruction *ins, const struct vkd3d_shader_location *location, + enum vkd3d_shader_opcode handler_idx) ```
If we're touching it anyway, we might as well rename it to vsir_instruction_init(), IMO.
```diff @@ -1062,7 +1068,10 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi }
if (!keep) - shader_instruction_init(ins, VKD3DSIH_NOP); + { + location = ins->location; + shader_instruction_init(ins, &location, VKD3DSIH_NOP); + } }
static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_shader_instruction_array *instructions,
```
This is not introduced by this series, so in that regard it's fine, but this looks odd. In particular: - Why shader_instruction_init(..., VKD3DSIH_NOP) instead of vkd3d_shader_instruction_make_nop()? - Why the "keep" variable if we could just nop out the instruction in the two places that might set it to false?
```diff +void vkd3d_shader_validate_vsir(struct vkd3d_shader_parser *parser) ```
I.e., vsir_validate()? :)
```diff +static void validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) VKD3D_PRINTF_FUNC(3, 4); + +static void validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, + const char *format, ...) +{ ```
I.e., ```c static void VKD3D_PRINTF_FUNC(3, 4) validator_error(struct validation_context *ctx, enum vkd3d_shader_error error, const char *format, ...) { ... ``` right?
```diff + case VKD3DSIH_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_HANDLER, "Invalid handler.\n"); ```
Note that vkd3d_shader_verror() already adds a newline, so validator_error() shouldn't need to add an extra one. That happens in a few other places in this series as well.
```diff +static void vkd3d_shader_validate_register(struct validation_context *ctx, + const struct vkd3d_shader_register *reg) +{ + switch (reg->type) + { + case VKD3DSPR_COUNT: + case VKD3DSPR_INVALID: + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE, "Invalid register type.\n"); + break; + + default: + break; + } +} ```
Should we just check for "reg->type >= VKD3DSPR_COUNT" and print the actual value here? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/317#note_44562