[PATCH v2 0/4] MR603: Draft: vkd3d-shader: Use 64 bit swizzles for 64 bit data types in VSIR.
The goal here is to be able to convert between SSA and TEMP easily, which will be the second preliminary pass for the CFG structurizer. Ideally I would have added the check in commit 4/4 for TEMPs as well, but typing information from TPF sources is insufficient. This leaves the current validation inconsistent: we check that a register is always used either as a scalar or a vec4, but that "scalar" might be 32 bit in some cases and 64 bit in others. Maybe the right thing to do here is just give up on trying to put some typing on TEMPs and just say they're always 4x32 bit words. I don't know, I'll leave this for when the dust is more settled. -- v2: vkd3d-shader/ir: Check that SSA registers are used with compatible data types. vkd3d-shader: Use 64 bit swizzles for 64 bit data types in VSIR. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> There are only three cases, and while the code is longer it is also hopefully easier to read. Moreover, an error message is casted if we're doing something unexpected. --- libs/vkd3d-shader/vkd3d_shader_private.h | 35 ++++++++++++++++++++---- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 51daf2153..c493bbcf4 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1599,15 +1599,40 @@ static inline unsigned int vkd3d_write_mask_from_component_count(unsigned int co static inline uint32_t vsir_write_mask_64_from_32(uint32_t write_mask32) { - uint32_t write_mask64 = write_mask32 | (write_mask32 >> 1); - return (write_mask64 & VKD3DSP_WRITEMASK_0) | ((write_mask64 & VKD3DSP_WRITEMASK_2) >> 1); + switch (write_mask32) + { + case VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_1: + return VKD3DSP_WRITEMASK_0; + + case VKD3DSP_WRITEMASK_2 | VKD3DSP_WRITEMASK_3: + return VKD3DSP_WRITEMASK_1; + + case VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_1 | VKD3DSP_WRITEMASK_2 | VKD3DSP_WRITEMASK_3: + return VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_1; + + default: + ERR("Invalid 32 bit writemask when converting to 64 bit: %#x.\n", write_mask32); + return VKD3DSP_WRITEMASK_0; + } } static inline uint32_t vsir_write_mask_32_from_64(uint32_t write_mask64) { - uint32_t write_mask32 = (write_mask64 | (write_mask64 << 1)) - & (VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_2); - return write_mask32 | (write_mask32 << 1); + switch (write_mask64) + { + case VKD3DSP_WRITEMASK_0: + return VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_1; + + case VKD3DSP_WRITEMASK_1: + return VKD3DSP_WRITEMASK_2 | VKD3DSP_WRITEMASK_3; + + case VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_1: + return VKD3DSP_WRITEMASK_0 | VKD3DSP_WRITEMASK_1 | VKD3DSP_WRITEMASK_2 | VKD3DSP_WRITEMASK_3; + + default: + ERR("Invalid 64 bit writemask: %#x.\n", write_mask64); + return VKD3DSP_WRITEMASK_0; + } } static inline unsigned int vsir_swizzle_get_component(uint32_t swizzle, unsigned int idx) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> So they can be used when a constant expression is expected, for instance on case labels. --- include/vkd3d_shader.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index a8cc3a336..2f4478a79 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -1777,10 +1777,10 @@ struct vkd3d_shader_dxbc_desc * \endcode */ #define VKD3D_SHADER_SWIZZLE(x, y, z, w) \ - vkd3d_shader_create_swizzle(VKD3D_SHADER_SWIZZLE_ ## x, \ - VKD3D_SHADER_SWIZZLE_ ## y, \ - VKD3D_SHADER_SWIZZLE_ ## z, \ - VKD3D_SHADER_SWIZZLE_ ## w) + (VKD3D_SHADER_SWIZZLE_ ## x << VKD3D_SHADER_SWIZZLE_SHIFT(0) \ + | VKD3D_SHADER_SWIZZLE_ ## y << VKD3D_SHADER_SWIZZLE_SHIFT(1) \ + | VKD3D_SHADER_SWIZZLE_ ## z << VKD3D_SHADER_SWIZZLE_SHIFT(2) \ + | VKD3D_SHADER_SWIZZLE_ ## w << VKD3D_SHADER_SWIZZLE_SHIFT(3)) /** The identity swizzle ".xyzw". */ #define VKD3D_SHADER_NO_SWIZZLE VKD3D_SHADER_SWIZZLE(X, Y, Z, W) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The handling of write masks and swizzles for 64 bit data types is currently irregular: write masks are always 64 bit, while swizzles are usually 32 bit, except for SSA registers with are 64 bit. With this change we always use 64 bit swizzles, in order to make the situation less surprising and make it easier to convert registers between SSA and TEMP. 64 bit swizzles are always required to have X in their last two components. --- libs/vkd3d-shader/d3d_asm.c | 17 ++++++--- libs/vkd3d-shader/dxil.c | 10 ++++- libs/vkd3d-shader/spirv.c | 31 ++++++++++------ libs/vkd3d-shader/tpf.c | 3 ++ libs/vkd3d-shader/vkd3d_shader_private.h | 47 ++++++++++++++++++++++-- 5 files changed, 86 insertions(+), 22 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 6ec7a9c99..2f54df41f 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1383,7 +1383,7 @@ static void shader_dump_dst_param(struct vkd3d_d3d_asm_compiler *compiler, { static const char write_mask_chars[] = "xyzw"; - if (param->reg.data_type == VKD3D_DATA_DOUBLE) + if (data_type_is_64_bit(param->reg.data_type)) write_mask = vsir_write_mask_32_from_64(write_mask); shader_addline(buffer, ".%s", compiler->colours.write_mask); @@ -1448,13 +1448,18 @@ static void shader_dump_src_param(struct vkd3d_d3d_asm_compiler *compiler, if (param->reg.type != VKD3DSPR_IMMCONST && param->reg.type != VKD3DSPR_IMMCONST64 && param->reg.dimension == VSIR_DIMENSION_VEC4) { - unsigned int swizzle_x = vsir_swizzle_get_component(swizzle, 0); - unsigned int swizzle_y = vsir_swizzle_get_component(swizzle, 1); - unsigned int swizzle_z = vsir_swizzle_get_component(swizzle, 2); - unsigned int swizzle_w = vsir_swizzle_get_component(swizzle, 3); - static const char swizzle_chars[] = "xyzw"; + unsigned int swizzle_x, swizzle_y, swizzle_z, swizzle_w; + + if (data_type_is_64_bit(param->reg.data_type)) + swizzle = vsir_swizzle_32_from_64(swizzle); + + swizzle_x = vsir_swizzle_get_component(swizzle, 0); + swizzle_y = vsir_swizzle_get_component(swizzle, 1); + swizzle_z = vsir_swizzle_get_component(swizzle, 2); + swizzle_w = vsir_swizzle_get_component(swizzle, 3); + if (swizzle_x == swizzle_y && swizzle_x == swizzle_z && swizzle_x == swizzle_w) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index c089d132c..cb6df8066 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -36,6 +36,10 @@ static const unsigned int SHADER_DESCRIPTOR_TYPE_COUNT = 4; static const unsigned int dx_max_thread_group_size[3] = {1024, 1024, 64}; +#define VKD3D_SHADER_SWIZZLE_64_MASK \ + (VKD3D_SHADER_SWIZZLE_MASK << VKD3D_SHADER_SWIZZLE_SHIFT(0) \ + | VKD3D_SHADER_SWIZZLE_MASK << VKD3D_SHADER_SWIZZLE_SHIFT(1)) + enum bitcode_block_id { BLOCKINFO_BLOCK = 0, @@ -2208,6 +2212,8 @@ static inline void src_param_init(struct vkd3d_shader_src_param *param) static void src_param_init_scalar(struct vkd3d_shader_src_param *param, unsigned int component_idx) { param->swizzle = vkd3d_shader_create_swizzle(component_idx, component_idx, component_idx, component_idx); + if (data_type_is_64_bit(param->reg.data_type)) + param->swizzle &= VKD3D_SHADER_SWIZZLE_64_MASK; param->modifiers = VKD3DSPSM_NONE; } @@ -3619,6 +3625,8 @@ static void sm6_parser_emit_dx_cbuffer_load(struct sm6_parser *sm6, enum dx_intr type = sm6_type_get_scalar_type(dst->type, 0); assert(type); src_param->reg.data_type = vkd3d_data_type_from_sm6_type(type); + if (data_type_is_64_bit(src_param->reg.data_type)) + src_param->swizzle = vsir_swizzle_64_from_32(src_param->swizzle); instruction_dst_param_init_ssa_vector(ins, sm6_type_max_vector_size(type), sm6); } @@ -4415,7 +4423,7 @@ static void sm6_parser_emit_extractval(struct sm6_parser *sm6, const struct dxil src_param = instruction_src_params_alloc(ins, 1, sm6); src_param_init_from_value(src_param, src); - src_param->swizzle = vkd3d_shader_create_swizzle(elem_idx, elem_idx, elem_idx, elem_idx); + src_param_init_scalar(src_param, elem_idx); instruction_dst_param_init_ssa_scalar(ins, sm6); } diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 299b4e965..03aabf66d 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -3594,11 +3594,17 @@ static bool vkd3d_swizzle_is_equal(uint32_t dst_write_mask, uint32_t swizzle, ui return vkd3d_compact_swizzle(VKD3D_SHADER_NO_SWIZZLE, dst_write_mask) == vkd3d_compact_swizzle(swizzle, write_mask); } -static bool vkd3d_swizzle_is_scalar(uint32_t swizzle) +static bool vkd3d_swizzle_is_scalar(uint32_t swizzle, const struct vkd3d_shader_register *reg) { unsigned int component_idx = vsir_swizzle_get_component(swizzle, 0); - return vsir_swizzle_get_component(swizzle, 1) == component_idx - && vsir_swizzle_get_component(swizzle, 2) == component_idx + + if (vsir_swizzle_get_component(swizzle, 1) != component_idx) + return false; + + if (data_type_is_64_bit(reg->data_type)) + return true; + + return vsir_swizzle_get_component(swizzle, 2) == component_idx && vsir_swizzle_get_component(swizzle, 3) == component_idx; } @@ -3719,7 +3725,7 @@ static uint32_t spirv_compiler_emit_load_constant64(struct spirv_compiler *compi for (i = 0, j = 0; i < VKD3D_DVEC2_SIZE; ++i) { if (write_mask & (VKD3DSP_WRITEMASK_0 << i)) - values[j++] = reg->u.immconst_u64[vsir_swizzle_get_component64(swizzle, i)]; + values[j++] = reg->u.immconst_u64[vsir_swizzle_get_component(swizzle, i)]; } } @@ -3886,7 +3892,7 @@ static uint32_t spirv_compiler_emit_load_ssa_reg(struct spirv_compiler *compiler assert(compiler->failed); return 0; } - assert(vkd3d_swizzle_is_scalar(swizzle)); + assert(vkd3d_swizzle_is_scalar(swizzle, reg)); if (reg->dimension == VSIR_DIMENSION_SCALAR) { @@ -3954,6 +3960,7 @@ static uint32_t spirv_compiler_emit_load_reg(struct spirv_compiler *compiler, val_id = vkd3d_spirv_build_op_load(builder, type_id, reg_info.id, SpvMemoryAccessMaskNone); } + swizzle = data_type_is_64_bit(reg->data_type) ? vsir_swizzle_32_from_64(swizzle) : swizzle; val_id = spirv_compiler_emit_swizzle(compiler, val_id, reg_info.write_mask, reg_info.component_type, swizzle, write_mask32); @@ -7043,11 +7050,11 @@ static void spirv_compiler_emit_ext_glsl_instruction(struct spirv_compiler *comp static void spirv_compiler_emit_mov(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { + uint32_t val_id, dst_val_id, type_id, dst_id, src_id, write_mask32, swizzle32; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; struct vkd3d_shader_register_info dst_reg_info, src_reg_info; const struct vkd3d_shader_dst_param *dst = instruction->dst; const struct vkd3d_shader_src_param *src = instruction->src; - uint32_t val_id, dst_val_id, type_id, dst_id, src_id; uint32_t components[VKD3D_VEC4_SIZE]; unsigned int i, component_count; @@ -7071,7 +7078,9 @@ static void spirv_compiler_emit_mov(struct spirv_compiler *compiler, return; } - component_count = vsir_write_mask_component_count(dst->write_mask); + write_mask32 = data_type_is_64_bit(dst->reg.data_type) ? vsir_write_mask_32_from_64(dst->write_mask) : dst->write_mask; + swizzle32 = data_type_is_64_bit(dst->reg.data_type) ? vsir_swizzle_32_from_64(src->swizzle) : src->swizzle; + component_count = vsir_write_mask_component_count(write_mask32); if (component_count != 1 && component_count != VKD3D_VEC4_SIZE && dst_reg_info.write_mask == VKD3DSP_WRITEMASK_ALL) { @@ -7084,8 +7093,8 @@ static void spirv_compiler_emit_mov(struct spirv_compiler *compiler, for (i = 0; i < ARRAY_SIZE(components); ++i) { - if (dst->write_mask & (VKD3DSP_WRITEMASK_0 << i)) - components[i] = VKD3D_VEC4_SIZE + vsir_swizzle_get_component(src->swizzle, i); + if (write_mask32 & (VKD3DSP_WRITEMASK_0 << i)) + components[i] = VKD3D_VEC4_SIZE + vsir_swizzle_get_component(swizzle32, i); else components[i] = i; } @@ -7815,7 +7824,7 @@ static void spirv_compiler_emit_branch(struct spirv_compiler *compiler, return; } - if (!vkd3d_swizzle_is_scalar(src->swizzle)) + if (!vkd3d_swizzle_is_scalar(src->swizzle, &src->reg)) { WARN("Unexpected src swizzle %#x.\n", src->swizzle); spirv_compiler_warning(compiler, VKD3D_SHADER_WARNING_SPV_INVALID_SWIZZLE, @@ -7842,7 +7851,7 @@ static void spirv_compiler_emit_switch(struct spirv_compiler *compiler, unsigned int i, word_count; uint32_t *cases; - if (!vkd3d_swizzle_is_scalar(src[0].swizzle)) + if (!vkd3d_swizzle_is_scalar(src[0].swizzle, &src[0].reg)) { WARN("Unexpected src swizzle %#x.\n", src[0].swizzle); spirv_compiler_warning(compiler, VKD3D_SHADER_WARNING_SPV_INVALID_SWIZZLE, diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 50146c2c7..e9fd39b88 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2161,6 +2161,9 @@ static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, cons break; } + if (data_type_is_64_bit(data_type)) + src_param->swizzle = vsir_swizzle_64_from_32(src_param->swizzle); + if (register_is_input_output(&src_param->reg) && !shader_sm4_validate_input_output_register(priv, &src_param->reg, mask_from_swizzle(src_param->swizzle))) return false; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index c493bbcf4..93bb57522 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -1635,14 +1635,53 @@ static inline uint32_t vsir_write_mask_32_from_64(uint32_t write_mask64) } } -static inline unsigned int vsir_swizzle_get_component(uint32_t swizzle, unsigned int idx) +static inline uint32_t vsir_swizzle_64_from_32(uint32_t swizzle32) { - return (swizzle >> VKD3D_SHADER_SWIZZLE_SHIFT(idx)) & VKD3D_SHADER_SWIZZLE_MASK; + switch (swizzle32) + { + case VKD3D_SHADER_SWIZZLE(X, Y, X, Y): + return VKD3D_SHADER_SWIZZLE(X, X, X, X); + + case VKD3D_SHADER_SWIZZLE(X, Y, Z, W): + return VKD3D_SHADER_SWIZZLE(X, Y, X, X); + + case VKD3D_SHADER_SWIZZLE(Z, W, X, Y): + return VKD3D_SHADER_SWIZZLE(Y, X, X, X); + + case VKD3D_SHADER_SWIZZLE(Z, W, Z, W): + return VKD3D_SHADER_SWIZZLE(Y, Y, X, X); + + default: + ERR("Invalid 32 bit swizzle when converting to 64 bit: %#x.\n", swizzle32); + return VKD3D_SHADER_SWIZZLE(X, X, X, X); + } } -static inline unsigned int vsir_swizzle_get_component64(uint32_t swizzle, unsigned int idx) +static inline uint32_t vsir_swizzle_32_from_64(uint32_t swizzle64) { - return ((swizzle >> VKD3D_SHADER_SWIZZLE_SHIFT(idx * 2)) & VKD3D_SHADER_SWIZZLE_MASK) / 2u; + switch (swizzle64) + { + case VKD3D_SHADER_SWIZZLE(X, X, X, X): + return VKD3D_SHADER_SWIZZLE(X, Y, X, Y); + + case VKD3D_SHADER_SWIZZLE(X, Y, X, X): + return VKD3D_SHADER_SWIZZLE(X, Y, Z, W); + + case VKD3D_SHADER_SWIZZLE(Y, X, X, X): + return VKD3D_SHADER_SWIZZLE(Z, W, X, Y); + + case VKD3D_SHADER_SWIZZLE(Y, Y, X, X): + return VKD3D_SHADER_SWIZZLE(Z, W, Z, W); + + default: + ERR("Invalid 64 bit swizzle: %#x.\n", swizzle64); + return VKD3D_SHADER_SWIZZLE(X, Y, X, Y); + } +} + +static inline unsigned int vsir_swizzle_get_component(uint32_t swizzle, unsigned int idx) +{ + return (swizzle >> VKD3D_SHADER_SWIZZLE_SHIFT(idx)) & VKD3D_SHADER_SWIZZLE_MASK; } static inline unsigned int vkd3d_compact_swizzle(uint32_t swizzle, uint32_t write_mask) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Specifically, accesses are always 32 bit or always 64 bit. --- libs/vkd3d-shader/ir.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index 45ccea45f..6b40d31ee 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -2431,6 +2431,7 @@ struct validation_context struct validation_context_ssa_data { enum vsir_dimension dimension; + enum vkd3d_data_type data_type; size_t first_seen; uint32_t write_mask; uint32_t read_mask; @@ -2587,13 +2588,20 @@ static void vsir_validate_register(struct validation_context *ctx, if (data->dimension == VSIR_DIMENSION_NONE) { data->dimension = reg->dimension; + data->data_type = reg->data_type; data->first_seen = ctx->instruction_idx; } - else if (data->dimension != reg->dimension) + else { - validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension %#x for a SSA register: " - "it has already been seen with dimension %#x at instruction %zu.", - reg->dimension, data->dimension, data->first_seen); + if (data->dimension != reg->dimension) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DIMENSION, "Invalid dimension %#x for a SSA register: " + "it has already been seen with dimension %#x at instruction %zu.", + reg->dimension, data->dimension, data->first_seen); + + if (data_type_is_64_bit(data->data_type) != data_type_is_64_bit(reg->data_type)) + validator_error(ctx, VKD3D_SHADER_ERROR_VSIR_INVALID_DATA_TYPE, "Invalid data type %#x for a SSA register: " + "it has already been seen with data type %#x at instruction %zu.", + reg->data_type, data->data_type, data->first_seen); } break; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603
I think I found the bit I was missing. Overall things are still pretty confusing and they might benefit from some general refactoring in which 64 bit support is designed from the get-go instead of added later, but this MR should at least be an improvement over the current situation, so I'm marking it as ready again. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603#note_59171
Conor McCarthy (@cmccarthy) commented about libs/vkd3d-shader/dxil.c:
src_param = instruction_src_params_alloc(ins, 1, sm6); src_param_init_from_value(src_param, src); - src_param->swizzle = vkd3d_shader_create_swizzle(elem_idx, elem_idx, elem_idx, elem_idx); + src_param_init_scalar(src_param, elem_idx);
Instead of the double initialisation, it may be better to call only `src_param_init_scalar()`, and then do `src_param->reg = src->u.reg` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/603#note_59304
participants (3)
-
Conor McCarthy (＠cmccarthy) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)