[PATCH v2 0/4] MR421: spirv: Remove some code no longer used after I/O normalization.
-- v2: vkd3d-shader/spirv: No longer handle builtin array size mismatch in spirv_compiler_emit_input(). vkd3d-shader/spirv: Remove handling of VKD3DSPR_OUTCONTROLPOINT. vkd3d-shader/ir: Check for vocp usage during IR normalization. vkd3d-shader/ir: Pass a vkd3d_shader_parser to instruction_array_normalise_io_registers(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/421
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 511b0e8fa..6a5fdc246 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1065,24 +1065,22 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi } } -static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_shader_instruction_array *instructions, - enum vkd3d_shader_type shader_type, struct shader_signature *input_signature, - struct shader_signature *output_signature, struct shader_signature *patch_constant_signature) +static enum vkd3d_result shader_normalise_io_registers(struct vkd3d_shader_parser *parser) { - struct io_normaliser normaliser = {*instructions}; + struct io_normaliser normaliser = {parser->instructions}; struct vkd3d_shader_instruction *ins; bool has_control_point_phase; unsigned int i, j; normaliser.phase = VKD3DSIH_INVALID; - normaliser.shader_type = shader_type; - normaliser.input_signature = input_signature; - normaliser.output_signature = output_signature; - normaliser.patch_constant_signature = patch_constant_signature; + normaliser.shader_type = parser->shader_version.type; + normaliser.input_signature = &parser->shader_desc.input_signature; + normaliser.output_signature = &parser->shader_desc.output_signature; + normaliser.patch_constant_signature = &parser->shader_desc.patch_constant_signature; - for (i = 0, has_control_point_phase = false; i < instructions->count; ++i) + for (i = 0, has_control_point_phase = false; i < parser->instructions.count; ++i) { - ins = &instructions->elements[i]; + ins = &parser->instructions.elements[i]; switch (ins->handler_idx) { @@ -1121,11 +1119,11 @@ static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_s } } - if (!shader_signature_merge(input_signature, normaliser.input_range_map, false) - || !shader_signature_merge(output_signature, normaliser.output_range_map, false) - || !shader_signature_merge(patch_constant_signature, normaliser.pc_range_map, true)) + if (!shader_signature_merge(&parser->shader_desc.input_signature, normaliser.input_range_map, false) + || !shader_signature_merge(&parser->shader_desc.output_signature, normaliser.output_range_map, false) + || !shader_signature_merge(&parser->shader_desc.patch_constant_signature, normaliser.pc_range_map, true)) { - *instructions = normaliser.instructions; + parser->instructions = normaliser.instructions; return VKD3D_ERROR_OUT_OF_MEMORY; } @@ -1133,7 +1131,7 @@ static enum vkd3d_result instruction_array_normalise_io_registers(struct vkd3d_s for (i = 0; i < normaliser.instructions.count; ++i) shader_instruction_normalise_io_params(&normaliser.instructions.elements[i], &normaliser); - *instructions = normaliser.instructions; + parser->instructions = normaliser.instructions; return VKD3D_OK; } @@ -1438,9 +1436,7 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, &parser->shader_desc.input_signature); } if (result >= 0) - result = instruction_array_normalise_io_registers(instructions, parser->shader_version.type, - &parser->shader_desc.input_signature, &parser->shader_desc.output_signature, - &parser->shader_desc.patch_constant_signature); + result = shader_normalise_io_registers(parser); if (result >= 0) result = instruction_array_normalise_flat_constants(parser); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/421
From: Zebediah Figura <zfigura(a)codeweavers.com> The hull shader barrier used for this was broken by I/O normalization, since vocp is no longer exposed to the spirv backend. Restore this barrier by checking for vocp during normalization instead. --- libs/vkd3d-shader/ir.c | 7 +++++++ libs/vkd3d-shader/spirv.c | 4 +--- libs/vkd3d-shader/vkd3d_shader_private.h | 2 ++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 6a5fdc246..b619b80b1 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -550,6 +550,8 @@ struct io_normaliser uint8_t input_range_map[MAX_REG_OUTPUT][VKD3D_VEC4_SIZE]; uint8_t output_range_map[MAX_REG_OUTPUT][VKD3D_VEC4_SIZE]; uint8_t pc_range_map[MAX_REG_OUTPUT][VKD3D_VEC4_SIZE]; + + bool use_vocp; }; static bool io_normaliser_is_in_fork_or_join_phase(const struct io_normaliser *normaliser) @@ -1025,6 +1027,10 @@ static void shader_instruction_normalise_io_params(struct vkd3d_shader_instructi if (normaliser->shader_type == VKD3D_SHADER_TYPE_HULL) { reg = &ins->declaration.dst.reg; + + if (reg->type == VKD3DSPR_OUTCONTROLPOINT) + normaliser->use_vocp = true; + /* We don't need to keep OUTCONTROLPOINT or PATCHCONST input declarations since their * equivalents were declared earlier, but INCONTROLPOINT may be the first occurrence. */ if (reg->type == VKD3DSPR_OUTCONTROLPOINT || reg->type == VKD3DSPR_PATCHCONST) @@ -1132,6 +1138,7 @@ static enum vkd3d_result shader_normalise_io_registers(struct vkd3d_shader_parse shader_instruction_normalise_io_params(&normaliser.instructions.elements[i], &normaliser); parser->instructions = normaliser.instructions; + parser->shader_desc.use_vocp = normaliser.use_vocp; return VKD3D_OK; } diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 2dab97ccb..b4bfe0873 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -6092,9 +6092,6 @@ static void spirv_compiler_emit_dcl_input(struct spirv_compiler *compiler, spirv_compiler_emit_input(compiler, dst, VKD3D_SIV_NONE, VKD3DSIM_NONE); else spirv_compiler_emit_input_register(compiler, dst); - - if (dst->reg.type == VKD3DSPR_OUTCONTROLPOINT) - compiler->use_vocp = true; } static void spirv_compiler_emit_dcl_input_ps(struct spirv_compiler *compiler, @@ -9693,6 +9690,7 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, memset(&shader_desc->input_signature, 0, sizeof(shader_desc->input_signature)); memset(&shader_desc->output_signature, 0, sizeof(shader_desc->output_signature)); memset(&shader_desc->patch_constant_signature, 0, sizeof(shader_desc->patch_constant_signature)); + compiler->use_vocp = parser->shader_desc.use_vocp; if (compiler->shader_type != VKD3D_SHADER_TYPE_HULL) spirv_compiler_emit_shader_signature_outputs(compiler); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index c9d2dec8b..08c88f227 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -933,6 +933,8 @@ struct vkd3d_shader_desc { uint32_t used, external; } flat_constant_count[3]; + + bool use_vocp; }; struct vkd3d_shader_register_semantic -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/421
From: Zebediah Figura <zfigura(a)codeweavers.com> I/O normalization removes this register type. --- libs/vkd3d-shader/spirv.c | 1 - libs/vkd3d-shader/vkd3d_shader_private.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index b4bfe0873..e627d5d9e 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4812,7 +4812,6 @@ static void spirv_compiler_emit_shader_phase_input(struct spirv_compiler *compil spirv_compiler_emit_input_register(compiler, dst); return; case VKD3DSPR_OUTPOINTID: /* Emitted in spirv_compiler_emit_initial_declarations(). */ - case VKD3DSPR_OUTCONTROLPOINT: /* See spirv_compiler_leave_shader_phase(). */ return; default: FIXME("Unhandled shader phase input register %#x.\n", reg->type); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 08c88f227..2cc412046 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1084,7 +1084,7 @@ static inline bool vkd3d_shader_instruction_has_texel_offset(const struct vkd3d_ static inline bool vkd3d_shader_register_is_input(const struct vkd3d_shader_register *reg) { - return reg->type == VKD3DSPR_INPUT || reg->type == VKD3DSPR_INCONTROLPOINT || reg->type == VKD3DSPR_OUTCONTROLPOINT; + return reg->type == VKD3DSPR_INPUT || reg->type == VKD3DSPR_INCONTROLPOINT; } static inline bool vkd3d_shader_register_is_output(const struct vkd3d_shader_register *reg) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/421
From: Zebediah Figura <zfigura(a)codeweavers.com> This is taken care of by prior I/O lowering. We no longer need to deal with this here. --- libs/vkd3d-shader/spirv.c | 53 ++++++++++++--------------------------- 1 file changed, 16 insertions(+), 37 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index e627d5d9e..b8bc1bb33 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4601,18 +4601,17 @@ static uint32_t spirv_compiler_emit_input(struct spirv_compiler *compiler, const struct signature_element *signature_element; const struct shader_signature *shader_signature; enum vkd3d_shader_component_type component_type; - uint32_t type_id, ptr_type_id, float_type_id; const struct vkd3d_spirv_builtin *builtin; unsigned int write_mask, reg_write_mask; struct vkd3d_symbol *symbol = NULL; uint32_t val_id, input_id, var_id; + uint32_t type_id, float_type_id; struct vkd3d_symbol reg_symbol; SpvStorageClass storage_class; struct rb_entry *entry = NULL; bool use_private_var = false; unsigned int array_sizes[2]; unsigned int element_idx; - uint32_t i, index; assert(!reg->idx_count || !reg->idx[0].rel_addr); assert(reg->idx_count < 2 || !reg->idx[1].rel_addr); @@ -4715,47 +4714,27 @@ static uint32_t spirv_compiler_emit_input(struct spirv_compiler *compiler, if (use_private_var) { + struct vkd3d_shader_register dst_reg = *reg; + dst_reg.data_type = VKD3D_DATA_FLOAT; + type_id = vkd3d_spirv_get_type_id(builder, component_type, input_component_count); - for (i = 0; i < max(array_sizes[0], 1); ++i) - { - struct vkd3d_shader_register dst_reg = *reg; - dst_reg.data_type = VKD3D_DATA_FLOAT; - val_id = input_id; - if (array_sizes[0]) - { - ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, SpvStorageClassInput, type_id); - index = spirv_compiler_get_constant_uint(compiler, i); - val_id = vkd3d_spirv_build_op_in_bounds_access_chain1(builder, ptr_type_id, input_id, index); - dst_reg.idx[0].offset = i; - } - else if (builtin && builtin->spirv_array_size) - { - /* The D3D builtin is not an array, but the SPIR-V builtin is, - * so we'll need to index into the SPIR-V builtin when loading - * it. This happens when reading TessLevel in domain shaders. */ - ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, SpvStorageClassInput, type_id); - index = spirv_compiler_get_constant_uint(compiler, builtin->member_idx); - val_id = vkd3d_spirv_build_op_in_bounds_access_chain1(builder, ptr_type_id, input_id, index); - dst_reg.idx[0].offset = element_idx + i; - } - val_id = vkd3d_spirv_build_op_load(builder, type_id, val_id, SpvMemoryAccessMaskNone); + val_id = vkd3d_spirv_build_op_load(builder, type_id, input_id, SpvMemoryAccessMaskNone); - if (builtin && builtin->fixup_pfn) - val_id = builtin->fixup_pfn(compiler, val_id); + if (builtin && builtin->fixup_pfn) + val_id = builtin->fixup_pfn(compiler, val_id); - if (component_type != VKD3D_SHADER_COMPONENT_FLOAT) - { - float_type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_FLOAT, input_component_count); - val_id = vkd3d_spirv_build_op_bitcast(builder, float_type_id, val_id); - } + if (component_type != VKD3D_SHADER_COMPONENT_FLOAT) + { + float_type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_FLOAT, input_component_count); + val_id = vkd3d_spirv_build_op_bitcast(builder, float_type_id, val_id); + } - val_id = spirv_compiler_emit_swizzle(compiler, val_id, - vkd3d_write_mask_from_component_count(input_component_count), - VKD3D_SHADER_COMPONENT_FLOAT, VKD3D_SHADER_NO_SWIZZLE, dst->write_mask >> component_idx); + val_id = spirv_compiler_emit_swizzle(compiler, val_id, + vkd3d_write_mask_from_component_count(input_component_count), + VKD3D_SHADER_COMPONENT_FLOAT, VKD3D_SHADER_NO_SWIZZLE, dst->write_mask >> component_idx); - spirv_compiler_emit_store_reg(compiler, &dst_reg, dst->write_mask, val_id); - } + spirv_compiler_emit_store_reg(compiler, &dst_reg, dst->write_mask, val_id); } return input_id; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/421
It's true that this is effectively unused now, but it's not clear to me that we have an equivalent barrier anywhere. I.e., does this being unused indicate a (potentially hard to debug) bug, instead of simply being some leftover code?
After struggling to understand tessellation again, I believe the answer is yes. I've pushed a new version that moves the check for vocp into the normalization pass and stores it in the vsir shader desc. Alternate approaches work, though, so let me know if you prefer a different way of detecting this case. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/421#note_50556
participants (2)
-
Zebediah Figura -
Zebediah Figura (＠zfigura)