[PATCH 0/3] MR574: vkd3d-shader/ir: Refactor VSIR passes.
From: Conor McCarthy <cmccarthy(a)codeweavers.com> We started with only one or two of these but it has become excessive. Patch originally written by Conor McCarthy and updated by Giovanni Mascellani. --- libs/vkd3d-shader/ir.c | 47 +++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index d6978171b..d4cd0fa14 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -2236,38 +2236,43 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, if (parser->shader_desc.is_dxil) return result; - if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL - && (result = remap_output_signature(parser, compile_info)) < 0) - return result; + if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL) + { + if ((result = remap_output_signature(parser, compile_info)) < 0) + return result; + } - if (parser->shader_version.type == VKD3D_SHADER_TYPE_HULL - && (result = instruction_array_flatten_hull_shader_phases(instructions)) >= 0) + if (parser->shader_version.type == VKD3D_SHADER_TYPE_HULL) { - result = instruction_array_normalise_hull_shader_control_point_io(instructions, - &parser->shader_desc.input_signature); + if ((result = instruction_array_flatten_hull_shader_phases(instructions)) < 0) + return result; + + if ((result = instruction_array_normalise_hull_shader_control_point_io(instructions, + &parser->shader_desc.input_signature)) < 0) + return result; } - if (result >= 0) - result = shader_normalise_io_registers(parser); - if (result >= 0) - result = instruction_array_normalise_flat_constants(parser); + if ((result = shader_normalise_io_registers(parser)) < 0) + return result; - if (result >= 0) - remove_dead_code(parser); + if ((result = instruction_array_normalise_flat_constants(parser)) < 0) + return result; - if (result >= 0) - result = flatten_control_flow_constructs(parser); + remove_dead_code(parser); - if (result >= 0) - result = normalise_combined_samplers(parser); + if ((result = flatten_control_flow_constructs(parser)) < 0) + return result; - if (result >= 0 && TRACE_ON()) + if ((result = normalise_combined_samplers(parser)) < 0) + return result; + + if (TRACE_ON()) vkd3d_shader_trace(instructions, &parser->shader_version); - if (result >= 0 && !parser->failed) - result = vsir_validate(parser); + if (!parser->failed && (result = vsir_validate(parser)) < 0) + return result; - if (result >= 0 && parser->failed) + if (parser->failed) result = VKD3D_ERROR_INVALID_SHADER; return result; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 48 +++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index d4cd0fa14..57783de68 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -2233,38 +2233,38 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction_array *instructions = &parser->instructions; enum vkd3d_result result = VKD3D_OK; - if (parser->shader_desc.is_dxil) - return result; - - if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL) + if (!parser->shader_desc.is_dxil) { - if ((result = remap_output_signature(parser, compile_info)) < 0) - return result; - } + if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL) + { + if ((result = remap_output_signature(parser, compile_info)) < 0) + return result; + } - if (parser->shader_version.type == VKD3D_SHADER_TYPE_HULL) - { - if ((result = instruction_array_flatten_hull_shader_phases(instructions)) < 0) - return result; + if (parser->shader_version.type == VKD3D_SHADER_TYPE_HULL) + { + if ((result = instruction_array_flatten_hull_shader_phases(instructions)) < 0) + return result; - if ((result = instruction_array_normalise_hull_shader_control_point_io(instructions, - &parser->shader_desc.input_signature)) < 0) - return result; - } + if ((result = instruction_array_normalise_hull_shader_control_point_io(instructions, + &parser->shader_desc.input_signature)) < 0) + return result; + } - if ((result = shader_normalise_io_registers(parser)) < 0) - return result; + if ((result = shader_normalise_io_registers(parser)) < 0) + return result; - if ((result = instruction_array_normalise_flat_constants(parser)) < 0) - return result; + if ((result = instruction_array_normalise_flat_constants(parser)) < 0) + return result; - remove_dead_code(parser); + remove_dead_code(parser); - if ((result = flatten_control_flow_constructs(parser)) < 0) - return result; + if ((result = flatten_control_flow_constructs(parser)) < 0) + return result; - if ((result = normalise_combined_samplers(parser)) < 0) - return result; + if ((result = normalise_combined_samplers(parser)) < 0) + return result; + } if (TRACE_ON()) vkd3d_shader_trace(instructions, &parser->shader_version); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> We have to do work to keep it updated across passes and we never read it. --- libs/vkd3d-shader/ir.c | 62 ++++++++++++--------------------------- libs/vkd3d-shader/spirv.c | 1 - 2 files changed, 18 insertions(+), 45 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 57783de68..150eae7fc 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -36,6 +36,19 @@ static void vkd3d_shader_instruction_make_nop(struct vkd3d_shader_instruction *i vsir_instruction_init(ins, &location, VKD3DSIH_NOP); } +static void remove_dcl_temps(struct vkd3d_shader_parser *parser) +{ + unsigned int i; + + for (i = 0; i < parser->instructions.count; ++i) + { + struct vkd3d_shader_instruction *ins = &parser->instructions.elements[i]; + + if (ins->handler_idx == VKD3DSIH_DCL_TEMPS) + vkd3d_shader_instruction_make_nop(ins); + } +} + static void shader_register_eliminate_phase_addressing(struct vkd3d_shader_register *reg, unsigned int instance_id) { @@ -143,9 +156,6 @@ struct hull_flattener { struct vkd3d_shader_instruction_array instructions; - unsigned int max_temp_count; - unsigned int temp_dcl_idx; - unsigned int instance_count; unsigned int phase_body_idx; enum vkd3d_shader_opcode phase; @@ -203,21 +213,6 @@ static void flattener_eliminate_phase_related_dcls(struct hull_flattener *normal vkd3d_shader_instruction_make_nop(ins); return; } - else if (ins->handler_idx == VKD3DSIH_DCL_TEMPS && normaliser->phase != VKD3DSIH_INVALID) - { - /* Leave only the first temp declaration and set it to the max count later. */ - if (!normaliser->max_temp_count) - { - normaliser->max_temp_count = ins->declaration.count; - normaliser->temp_dcl_idx = index; - } - else - { - normaliser->max_temp_count = max(normaliser->max_temp_count, ins->declaration.count); - vkd3d_shader_instruction_make_nop(ins); - } - return; - } if (normaliser->phase == VKD3DSIH_INVALID || shader_instruction_is_dcl(ins)) return; @@ -371,9 +366,6 @@ static enum vkd3d_result instruction_array_flatten_hull_shader_phases(struct vkd if (flattener.phase != VKD3DSIH_INVALID) { - if (flattener.temp_dcl_idx) - instructions->elements[flattener.temp_dcl_idx].declaration.count = flattener.max_temp_count; - if (!shader_instruction_array_reserve(&flattener.instructions, flattener.instructions.count + 1)) return VKD3D_ERROR_OUT_OF_MEMORY; vsir_instruction_init(&instructions->elements[instructions->count++], &flattener.last_ret_location, VKD3DSIH_RET); @@ -570,9 +562,6 @@ struct io_normaliser struct shader_signature *output_signature; struct shader_signature *patch_constant_signature; - unsigned int max_temp_count; - unsigned int temp_dcl_idx; - unsigned int instance_count; unsigned int phase_body_idx; enum vkd3d_shader_opcode phase; @@ -2233,6 +2222,8 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, struct vkd3d_shader_instruction_array *instructions = &parser->instructions; enum vkd3d_result result = VKD3D_OK; + remove_dcl_temps(parser); + if (!parser->shader_desc.is_dxil) { if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL) @@ -2283,8 +2274,6 @@ struct validation_context struct vkd3d_shader_parser *parser; size_t instruction_idx; bool invalid_instruction_idx; - bool dcl_temps_found; - unsigned int temp_count; enum vkd3d_shader_opcode phase; struct validation_context_temp_data @@ -2339,11 +2328,7 @@ static void vsir_validate_src_param(struct validation_context *ctx, static void vsir_validate_register(struct validation_context *ctx, const struct vkd3d_shader_register *reg) { - unsigned int i, temp_count = ctx->temp_count; - - /* SM1-3 shaders do not include a DCL_TEMPS instruction. */ - if (ctx->parser->shader_version.major <= 3) - temp_count = ctx->parser->shader_desc.temp_count; + unsigned int i; if (reg->type >= VKD3DSPR_COUNT) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_REGISTER_TYPE, "Invalid register type %#x.", @@ -2388,18 +2373,13 @@ static void vsir_validate_register(struct validation_context *ctx, if (reg->idx[0].rel_addr) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "Non-NULL relative address for a TEMP register."); - if (reg->idx[0].offset >= temp_count) + if (reg->idx[0].offset >= ctx->parser->shader_desc.temp_count) { validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INDEX, "TEMP register index %u exceeds the maximum count %u.", - reg->idx[0].offset, temp_count); + reg->idx[0].offset, ctx->parser->shader_desc.temp_count); break; } - /* parser->shader_desc.temp_count might be smaller then - * temp_count if the parser made a mistake; we still don't - * want to overflow the array. */ - if (reg->idx[0].offset >= ctx->parser->shader_desc.temp_count) - break; data = &ctx->temps[reg->idx[0].offset]; if (reg->dimension == VSIR_DIMENSION_NONE) @@ -2640,8 +2620,6 @@ static void vsir_validate_instruction(struct validation_context *ctx) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_INSTRUCTION_NESTING, "Phase instruction %#x must appear to top level.", instruction->handler_idx); ctx->phase = instruction->handler_idx; - ctx->dcl_temps_found = false; - ctx->temp_count = 0; return; default: @@ -2658,13 +2636,9 @@ static void vsir_validate_instruction(struct validation_context *ctx) case VKD3DSIH_DCL_TEMPS: vsir_validate_dst_count(ctx, instruction, 0); vsir_validate_src_count(ctx, instruction, 0); - if (ctx->dcl_temps_found) - validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_DUPLICATE_DCL_TEMPS, "Duplicate DCL_TEMPS instruction."); if (instruction->declaration.count > ctx->parser->shader_desc.temp_count) validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DCL_TEMPS, "Invalid DCL_TEMPS count %u, expected at most %u.", instruction->declaration.count, ctx->parser->shader_desc.temp_count); - ctx->dcl_temps_found = true; - ctx->temp_count = instruction->declaration.count; break; case VKD3DSIH_IF: diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 5798f262c..9b6cf6133 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9577,7 +9577,6 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_DCL_RESOURCE_RAW: case VKD3DSIH_DCL_RESOURCE_STRUCTURED: case VKD3DSIH_DCL_SAMPLER: - case VKD3DSIH_DCL_TEMPS: case VKD3DSIH_DCL_UAV_RAW: case VKD3DSIH_DCL_UAV_STRUCTURED: case VKD3DSIH_DCL_UAV_TYPED: -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574
Conor McCarthy (@cmccarthy) commented about libs/vkd3d-shader/ir.c:
struct vkd3d_shader_instruction_array *instructions = &parser->instructions; enum vkd3d_result result = VKD3D_OK;
- if (parser->shader_desc.is_dxil) - return result; - - if (parser->shader_version.type != VKD3D_SHADER_TYPE_PIXEL) + if (!parser->shader_desc.is_dxil)
I have an alternate version of this in !579, using separate `vkd3d_shader_normalise_sm4()` and `vkd3d_shader_normalise_sm6()` per your suggestion. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58109
On Fri Jan 19 01:29:18 2024 +0000, Conor McCarthy wrote:
I have an alternate version of this in !579, using separate `vkd3d_shader_normalise_sm4()` and `vkd3d_shader_normalise_sm6()` per your suggestion. Yeah, after some experiments I thought this might have been a better alternative. But I'm not terribly attached to either (and either looks better than the status quo to me). Votes welcome.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58160
Votes welcome.
I don't hate either version. I imagine that ultimately we may want to move to a scheme like this though: ```c if (program->cf_type == CF_TYPE_STRUCTURED) flatten_control_flow_constructs(program); [...] ``` where "cf_type" would originally be set by the parser. I.e., explicitly checking for properties of the vsir instead of having those be implied by the shader model or bytecode format. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58448
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/ir.c:
case VKD3DSIH_DCL_TEMPS: vsir_validate_dst_count(ctx, instruction, 0); vsir_validate_src_count(ctx, instruction, 0); - if (ctx->dcl_temps_found) - validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_DUPLICATE_DCL_TEMPS, "Duplicate DCL_TEMPS instruction.");
It is necessary to remove `dcl_temps_found`? It may still be useful to detect duplicated DCL_TEMPS instructions. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58465
On Fri Jan 19 10:06:51 2024 +0000, Giovanni Mascellani wrote:
Yeah, after some experiments I thought this might have been a better alternative. But I'm not terribly attached to either (and either looks better than the status quo to me). Votes welcome. The fact that we can potentially have passes that should be called on both `vkd3d_shader_normalise_sm4()` and `vkd3d_shader_normalise_sm6()`, such as `remove_dcl_temps()` in 3/3 may be an argument against separating the function, but I don't know if we will end up with enough for the repetition to be a problem.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58474
I don't hate either version. I imagine that ultimately we may want to move to a scheme like this though:
```c if (program->cf_type == CF_TYPE_STRUCTURED) flatten_control_flow_constructs(program); [...] ```
where "cf_type" would originally be set by the parser. I.e., explicitly checking for properties of the vsir instead of having those be implied by the shader model or bytecode format.
Yes, that's my idea too. Relatedly, as was already discussed, I expect support for different dialects/flags to eventually appear in order to validate language features that do not necessarily hold at every step, but which are still worthwhile to check at every step where they should hold. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58478
On Mon Jan 22 20:31:31 2024 +0000, Francisco Casas wrote:
It is necessary to remove `dcl_temps_found`? It may still be useful to detect duplicated DCL_TEMPS instructions. In theory yes, but for the moment I mostly see the validator as a device to ease development (by providing a reference on the VSIR syntax and therefore an interface between frontend, passes and backend) rather than a picky language checker for the sake of adhering to a specific standard. Since basically developing passes and backends is much easier using `temp_count`, I currently see checking specifically that DCL_TEMPS is not duplicate as out of scope.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58480
The fact that we can potentially have passes that should be called on both `vkd3d_shader_normalise_sm4()` and `vkd3d_shader_normalise_sm6()`, such as `remove_dcl_temps()` in 3/3 may be an argument against separating the function, but I don't know if we will end up with enough for the repetition to be a problem.
Yeah, I guess it's still a bit early to commit on that decision. Right now both alternatives look essentially equivalent. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/574#note_58482
participants (6)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Francisco Casas (＠fcasas)
-
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)