[PATCH 0/1] MR442: vkd3d-shader/ir: Normalise opcode double variants to their equivalent float variants.
Instructions such as DMOV, DADD, DEQ, etc. are redundant because parameter width is specified by register data type. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Instructions such as DMOV, DADD, DEQ, etc. are redundant because parameter width is specified by register data type. --- libs/vkd3d-shader/ir.c | 144 +++++++++++++++++++++++ libs/vkd3d-shader/spirv.c | 61 ++++------ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 3 files changed, 167 insertions(+), 39 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 8cf92656b..9d15d148e 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1424,6 +1424,147 @@ static enum vkd3d_result normalise_combined_samplers(struct vkd3d_shader_parser return VKD3D_OK; } +static void src_param_validate_is_double(const struct vkd3d_shader_src_param *src, + enum vkd3d_shader_opcode handler_idx, struct vkd3d_shader_parser *parser) +{ + if (src->reg.data_type != VKD3D_DATA_DOUBLE) + { + WARN("Invalid data type %#x.\n", src->reg.data_type); + vkd3d_shader_parser_error(parser, VKD3D_SHADER_ERROR_VSIR_INVALID_DATA_TYPE, + "Source parameter type %#x for instruction handler %#x is not double.", + src->reg.data_type, handler_idx); + } +} + +static void dst_param_validate_is_double(const struct vkd3d_shader_dst_param *dst, + enum vkd3d_shader_opcode handler_idx, struct vkd3d_shader_parser *parser) +{ + if (dst->reg.data_type != VKD3D_DATA_DOUBLE) + { + WARN("Invalid data type %#x.\n", dst->reg.data_type); + vkd3d_shader_parser_error(parser, VKD3D_SHADER_ERROR_VSIR_INVALID_DATA_TYPE, + "Destination parameter type %#x for instruction handler %#x is not double.", + dst->reg.data_type, handler_idx); + } +} + +static void instruction_validate_src_params_are_double(const struct vkd3d_shader_instruction *ins, + struct vkd3d_shader_parser *parser) +{ + unsigned int i; + for (i = 0; i < ins->src_count; ++i) + src_param_validate_is_double(&ins->src[i], ins->handler_idx, parser); +} + +static void instruction_validate_dst_params_are_double(const struct vkd3d_shader_instruction *ins, + struct vkd3d_shader_parser *parser) +{ + unsigned int i; + for (i = 0; i < ins->dst_count; ++i) + dst_param_validate_is_double(&ins->dst[i], ins->handler_idx, parser); +} + +static void instruction_validate_all_params_are_double(const struct vkd3d_shader_instruction *ins, + struct vkd3d_shader_parser *parser) +{ + instruction_validate_src_params_are_double(ins, parser); + instruction_validate_dst_params_are_double(ins, parser); +} + +static void normalise_double_opcode_variants(struct vkd3d_shader_parser *parser) +{ + unsigned int i, j; + + for (i = 0; i < parser->instructions.count; ++i) + { + struct vkd3d_shader_instruction *ins = &parser->instructions.elements[i]; + + switch (ins->handler_idx) + { + case VKD3DSIH_DMOV: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_MOV; + break; + case VKD3DSIH_DMOVC: + for (j = 1; j < ins->src_count; ++j) + src_param_validate_is_double(&ins->src[j], ins->handler_idx, parser); + instruction_validate_dst_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_MOVC; + break; + case VKD3DSIH_DADD: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_ADD; + break; + case VKD3DSIH_DDIV: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_DIV; + break; + case VKD3DSIH_DMUL: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_MUL; + break; + case VKD3DSIH_DTOF: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_FTOF; + break; + case VKD3DSIH_DTOI: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_FTOI; + break; + case VKD3DSIH_DTOU: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_FTOU; + break; + case VKD3DSIH_FTOD: + instruction_validate_dst_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_FTOF; + break; + case VKD3DSIH_ITOD: + instruction_validate_dst_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_ITOF; + break; + case VKD3DSIH_UTOD: + instruction_validate_dst_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_UTOF; + break; + case VKD3DSIH_DFMA: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_MAD; + break; + case VKD3DSIH_DMAX: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_MAX; + break; + case VKD3DSIH_DMIN: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_MIN; + break; + case VKD3DSIH_DRCP: + instruction_validate_all_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_RCP; + break; + case VKD3DSIH_DEQ: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_EQ; + break; + case VKD3DSIH_DGE: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_GE; + break; + case VKD3DSIH_DLT: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_LT; + break; + case VKD3DSIH_DNE: + instruction_validate_src_params_are_double(ins, parser); + ins->handler_idx = VKD3DSIH_NE; + break; + default: + break; + } + } +} + enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, const struct vkd3d_shader_compile_info *compile_info) { @@ -1457,6 +1598,9 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, if (result >= 0) result = normalise_combined_samplers(parser); + if (result >= 0) + normalise_double_opcode_variants(parser); + if (result >= 0 && TRACE_ON()) vkd3d_shader_trace(instructions, &parser->shader_version); diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 6faac0ccd..b8b19c69d 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -6595,25 +6595,17 @@ static SpvOp spirv_compiler_map_alu_instruction(const struct vkd3d_shader_instru {VKD3DSIH_AND, SpvOpBitwiseAnd}, {VKD3DSIH_BFREV, SpvOpBitReverse}, {VKD3DSIH_COUNTBITS, SpvOpBitCount}, - {VKD3DSIH_DADD, SpvOpFAdd}, - {VKD3DSIH_DDIV, SpvOpFDiv}, {VKD3DSIH_DIV, SpvOpFDiv}, - {VKD3DSIH_DMUL, SpvOpFMul}, - {VKD3DSIH_DTOF, SpvOpFConvert}, - {VKD3DSIH_DTOI, SpvOpConvertFToS}, - {VKD3DSIH_DTOU, SpvOpConvertFToU}, - {VKD3DSIH_FTOD, SpvOpFConvert}, + {VKD3DSIH_FTOF, SpvOpFConvert}, {VKD3DSIH_IADD, SpvOpIAdd}, {VKD3DSIH_INEG, SpvOpSNegate}, {VKD3DSIH_ISHL, SpvOpShiftLeftLogical}, {VKD3DSIH_ISHR, SpvOpShiftRightArithmetic}, - {VKD3DSIH_ITOD, SpvOpConvertSToF}, {VKD3DSIH_ITOF, SpvOpConvertSToF}, {VKD3DSIH_MUL, SpvOpFMul}, {VKD3DSIH_NOT, SpvOpNot}, {VKD3DSIH_OR, SpvOpBitwiseOr}, {VKD3DSIH_USHR, SpvOpShiftRightLogical}, - {VKD3DSIH_UTOD, SpvOpConvertUToF}, {VKD3DSIH_UTOF, SpvOpConvertUToF}, {VKD3DSIH_XOR, SpvOpBitwiseXor}, }; @@ -6687,9 +6679,6 @@ static enum GLSLstd450 spirv_compiler_map_ext_glsl_instruction( } glsl_insts[] = { - {VKD3DSIH_DFMA, GLSLstd450Fma}, - {VKD3DSIH_DMAX, GLSLstd450NMax}, - {VKD3DSIH_DMIN, GLSLstd450NMin}, {VKD3DSIH_EXP, GLSLstd450Exp2}, {VKD3DSIH_FIRSTBIT_HI, GLSLstd450FindUMsb}, {VKD3DSIH_FIRSTBIT_LO, GLSLstd450FindILsb}, @@ -7091,10 +7080,18 @@ static void spirv_compiler_emit_ftoi(struct spirv_compiler *compiler, dst_type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); - int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, component_count); + if (src->reg.data_type == VKD3D_DATA_DOUBLE) + { + int_min_id = spirv_compiler_get_constant_double_vector(compiler, -2147483648.0, component_count); + float_max_id = spirv_compiler_get_constant_double_vector(compiler, 2147483648.0, component_count); + } + else + { + int_min_id = spirv_compiler_get_constant_float_vector(compiler, -2147483648.0f, component_count); + float_max_id = spirv_compiler_get_constant_float_vector(compiler, 2147483648.0f, component_count); + } val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, int_min_id); - float_max_id = spirv_compiler_get_constant_float_vector(compiler, 2147483648.0f, component_count); int_max_id = spirv_compiler_get_constant_int_vector(compiler, INT_MAX, component_count); condition_type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_BOOL, component_count); condition_id = vkd3d_spirv_build_op_tr2(builder, &builder->function_stream, @@ -7132,10 +7129,18 @@ static void spirv_compiler_emit_ftou(struct spirv_compiler *compiler, dst_type_id = spirv_compiler_get_type_id_for_dst(compiler, dst); src_id = spirv_compiler_emit_load_src(compiler, src, dst->write_mask); - zero_id = spirv_compiler_get_constant_float_vector(compiler, 0.0f, component_count); + if (src->reg.data_type == VKD3D_DATA_DOUBLE) + { + zero_id = spirv_compiler_get_constant_double_vector(compiler, 0.0, component_count); + float_max_id = spirv_compiler_get_constant_double_vector(compiler, 4294967296.0, component_count); + } + else + { + zero_id = spirv_compiler_get_constant_float_vector(compiler, 0.0f, component_count); + float_max_id = spirv_compiler_get_constant_float_vector(compiler, 4294967296.0f, component_count); + } val_id = vkd3d_spirv_build_op_glsl_std450_max(builder, src_type_id, src_id, zero_id); - float_max_id = spirv_compiler_get_constant_float_vector(compiler, 4294967296.0f, component_count); uint_max_id = spirv_compiler_get_constant_uint_vector(compiler, UINT_MAX, component_count); condition_type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_BOOL, component_count); condition_id = vkd3d_spirv_build_op_tr2(builder, &builder->function_stream, @@ -7282,17 +7287,13 @@ static void spirv_compiler_emit_comparison_instruction(struct spirv_compiler *co switch (instruction->handler_idx) { - case VKD3DSIH_DEQ: case VKD3DSIH_EQ: op = SpvOpFOrdEqual; break; - case VKD3DSIH_DGE: case VKD3DSIH_GE: op = SpvOpFOrdGreaterThanEqual; break; case VKD3DSIH_IEQ: op = SpvOpIEqual; break; case VKD3DSIH_IGE: op = SpvOpSGreaterThanEqual; break; case VKD3DSIH_ILT: op = SpvOpSLessThan; break; case VKD3DSIH_INE: op = SpvOpINotEqual; break; - case VKD3DSIH_DLT: case VKD3DSIH_LT: op = SpvOpFOrdLessThan; break; - case VKD3DSIH_DNE: case VKD3DSIH_NE: op = SpvOpFUnordNotEqual; break; case VKD3DSIH_UGE: op = SpvOpUGreaterThanEqual; break; case VKD3DSIH_ULT: op = SpvOpULessThan; break; @@ -9391,11 +9392,9 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_HS_JOIN_PHASE: spirv_compiler_enter_shader_phase(compiler, instruction); break; - case VKD3DSIH_DMOV: case VKD3DSIH_MOV: spirv_compiler_emit_mov(compiler, instruction); break; - case VKD3DSIH_DMOVC: case VKD3DSIH_MOVC: spirv_compiler_emit_movc(compiler, instruction); break; @@ -9406,32 +9405,21 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_AND: case VKD3DSIH_BFREV: case VKD3DSIH_COUNTBITS: - case VKD3DSIH_DADD: - case VKD3DSIH_DDIV: case VKD3DSIH_DIV: - case VKD3DSIH_DMUL: - case VKD3DSIH_DTOF: - case VKD3DSIH_DTOI: - case VKD3DSIH_DTOU: - case VKD3DSIH_FTOD: + case VKD3DSIH_FTOF: case VKD3DSIH_IADD: case VKD3DSIH_INEG: case VKD3DSIH_ISHL: case VKD3DSIH_ISHR: - case VKD3DSIH_ITOD: case VKD3DSIH_ITOF: case VKD3DSIH_MUL: case VKD3DSIH_NOT: case VKD3DSIH_OR: case VKD3DSIH_USHR: - case VKD3DSIH_UTOD: case VKD3DSIH_UTOF: case VKD3DSIH_XOR: spirv_compiler_emit_alu_instruction(compiler, instruction); break; - case VKD3DSIH_DFMA: - case VKD3DSIH_DMAX: - case VKD3DSIH_DMIN: case VKD3DSIH_EXP: case VKD3DSIH_FIRSTBIT_HI: case VKD3DSIH_FIRSTBIT_LO: @@ -9458,7 +9446,6 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_DP2: spirv_compiler_emit_dot(compiler, instruction); break; - case VKD3DSIH_DRCP: case VKD3DSIH_RCP: spirv_compiler_emit_rcp(compiler, instruction); break; @@ -9480,10 +9467,6 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler, case VKD3DSIH_FTOU: spirv_compiler_emit_ftou(compiler, instruction); break; - case VKD3DSIH_DEQ: - case VKD3DSIH_DGE: - case VKD3DSIH_DLT: - case VKD3DSIH_DNE: case VKD3DSIH_EQ: case VKD3DSIH_GE: case VKD3DSIH_IEQ: diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 912267e0e..81227758a 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -337,6 +337,7 @@ enum vkd3d_shader_opcode VKD3DSIH_FIRSTBIT_SHI, VKD3DSIH_FRC, VKD3DSIH_FTOD, + VKD3DSIH_FTOF, VKD3DSIH_FTOI, VKD3DSIH_FTOU, VKD3DSIH_GATHER4, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442
An alternative is to still handle the double opcode variants, and change `emit_ftoi()` and `emit_ftou()` to support `DTOI` and `DTOU`, but do we want the backend to handle some instructions which do exactly the same thing as some other instructions? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442#note_50990
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442
In principle this seems fine, but from this MR on its own it's not obvious to me that this is worth it; the amount of code we're adding for the lowering pass is quite a bit more than what we're removing from the SPIR-V backend. Part of that seems to come from the validation code, which would perhaps more properly be part of vsir_validate_instruction() to begin with. Beyond that though, I imagine part of the motivation is also simplifying the DXIL parser, right? How much does this help there? I imagine we'll also need some changes to the d3d-asm output in order to distinguish between e.g. ADD on VKD3D_DATA_FLOAT sources and ADD on VKD3D_DATA_DOUBLE sources. That's probably not much of a concern right now (although it does make the post-normalisation trace slightly awkward), but that would change once the DXIL parser starts emitting these. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442#note_51029
Beyond that though, I imagine part of the motivation is also simplifying the DXIL parser, right? How much does this help there?
I'm also doubting its worth, because if we modify `emit_ftoi()` and `emit_ftou()`, the DXIL parser doesn't need to emit double instruction variants at all.
I imagine we'll also need some changes to the d3d-asm output in order to distinguish between e.g. ADD on VKD3D_DATA_FLOAT sources and ADD on VKD3D_DATA_DOUBLE sources.
I was thinking of doing this where double is used with non-double instructions. If we don't normalise TPF it would only change the disassembly of DXIL shaders. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442#note_51047
I imagine we'll also need some changes to the d3d-asm output in order to distinguish between e.g. ADD on VKD3D_DATA_FLOAT sources and ADD on VKD3D_DATA_DOUBLE sources.
I was thinking of doing this where double is used with non-double instructions. If we don't normalise TPF it would only change the disassembly of DXIL shaders.
Right. I guess the question is what that would look like. One approach would be to output ADD with VKD3D_DATA_DOUBLE as DADD. At that point we could arguably just as well just emit a DADD in the first place from the DXIL parser though. Another approach would be to just include type information in the output for shader model 6+, much like DXIL assembly does. There would be some choices to make there as well; e.g., we could always switch this based on the shader model, but we could also make this a formatting option, which would allow displaying these for e.g. TPF shaders as well. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442#note_51104
Right. I guess the question is what that would look like. One approach would be to output ADD with VKD3D_DATA_DOUBLE as DADD. At that point we could arguably just as well just emit a DADD in the first place from the DXIL parser though. Another approach would be to just include type information in the output for shader model 6+, much like DXIL assembly does. There would be some choices to make there as well; e.g., we could always switch this based on the shader model, but we could also make this a formatting option, which would allow displaying these for e.g. TPF shaders as well.
In the interest of making it easier to use VSIR as an IR, it would certainly be useful to have a way to dump it in a way which doesn't have too much of a TPF-colored lens on it. I.e., some typing information, a more regular printout of indices; I might be wrong, but I guess that current the disassembler only prints instruction/register/index combinations which make sense in D3DBC/TPF, while we probably would like to have more freedom on that at some point. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442#note_51300
In the interest of making it easier to use VSIR as an IR, it would certainly be useful to have a way to dump it in a way which doesn't have too much of a TPF-colored lens on it. I.e., some typing information, a more regular printout of indices; I might be wrong, but I guess that current the disassembler only prints instruction/register/index combinations which make sense in D3DBC/TPF, while we probably would like to have more freedom on that at some point.
Yeah, I agree. I imagine we could get there by tweaking the d3d-asm output somewhat for internal use. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442#note_51378
This merge request was closed by Conor McCarthy. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/442
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)