[PATCH v3 0/2] MR225: vkd3d-shader/tpf: Read complete swizzle/mask info from parameter tokens.
-- v3: vkd3d-shader/tpf: Read complete swizzle/mask info for dst params. vkd3d-shader/tpf: Read complete swizzle/mask info for src params. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225
From: Conor McCarthy <cmccarthy(a)codeweavers.com> VKD3D_SM4_SWIZZLE_NONE is incorrect; the token contains a mask in this case. Reading the mask allows the correct swizzle to be set, detection of unexpected mask values, and factors out shader_sm4_is_scalar_register() which is somewhat of a hack. --- libs/vkd3d-shader/spirv.c | 2 +- libs/vkd3d-shader/tpf.c | 81 ++++++++++++++++++++++++--------------- 2 files changed, 52 insertions(+), 31 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index cc0b63e82..346a95925 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7390,7 +7390,7 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c assert(compiler->control_flow_depth); assert(cf_info->current_block == VKD3D_BLOCK_SWITCH); - assert(src->swizzle == VKD3D_SHADER_NO_SWIZZLE && src->reg.type == VKD3DSPR_IMMCONST); + assert(src->swizzle == VKD3D_SHADER_SWIZZLE(X, X, X, X) && src->reg.type == VKD3DSPR_IMMCONST); value = *src->reg.u.immconst_uint; if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.case_blocks, &cf_info->u.switch_.case_blocks_size, diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 60948d649..a7ad5f1de 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -505,7 +505,7 @@ enum vkd3d_sm4_input_primitive_type enum vkd3d_sm4_swizzle_type { - VKD3D_SM4_SWIZZLE_NONE = 0x0, + VKD3D_SM4_SWIZZLE_MASK4 = 0x0, VKD3D_SM4_SWIZZLE_VEC4 = 0x1, VKD3D_SM4_SWIZZLE_SCALAR = 0x2, }; @@ -1953,6 +1953,7 @@ static bool shader_sm4_validate_input_output_register(struct vkd3d_shader_sm4_pa static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, const uint32_t **ptr, const uint32_t *end, enum vkd3d_data_type data_type, struct vkd3d_shader_src_param *src_param) { + unsigned int dimension, mask; DWORD token; if (*ptr >= end) @@ -1968,37 +1969,57 @@ static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, cons return false; } - if (src_param->reg.type == VKD3DSPR_IMMCONST || src_param->reg.type == VKD3DSPR_IMMCONST64) + switch ((dimension = (token & VKD3D_SM4_DIMENSION_MASK) >> VKD3D_SM4_DIMENSION_SHIFT)) { - src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; - } - else - { - enum vkd3d_sm4_swizzle_type swizzle_type = - (token & VKD3D_SM4_SWIZZLE_TYPE_MASK) >> VKD3D_SM4_SWIZZLE_TYPE_SHIFT; + case VKD3D_SM4_DIMENSION_NONE: + src_param->swizzle = 0; + break; - switch (swizzle_type) + case VKD3D_SM4_DIMENSION_SCALAR: + src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); + break; + + case VKD3D_SM4_DIMENSION_VEC4: { - case VKD3D_SM4_SWIZZLE_NONE: - if (shader_sm4_is_scalar_register(&src_param->reg)) - src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); - else + enum vkd3d_sm4_swizzle_type swizzle_type = + (token & VKD3D_SM4_SWIZZLE_TYPE_MASK) >> VKD3D_SM4_SWIZZLE_TYPE_SHIFT; + + switch (swizzle_type) + { + case VKD3D_SM4_SWIZZLE_MASK4: + mask = (token & VKD3D_SM4_WRITEMASK_MASK) >> VKD3D_SM4_WRITEMASK_SHIFT; + src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; - break; + if (mask == VKD3DSP_WRITEMASK_0) + src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); + else if (!mask) + src_param->swizzle = 0; + else if (mask != VKD3DSP_WRITEMASK_ALL) + FIXME("Unhandled mask %#x.\n", mask); + + if (!mask && (src_param->reg.type == VKD3DSPR_IMMCONST || src_param->reg.type == VKD3DSPR_IMMCONST64)) + src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; + break; - case VKD3D_SM4_SWIZZLE_SCALAR: - src_param->swizzle = (token & VKD3D_SM4_SWIZZLE_MASK) >> VKD3D_SM4_SWIZZLE_SHIFT; - src_param->swizzle = (src_param->swizzle & 0x3) * 0x01010101; - break; + case VKD3D_SM4_SWIZZLE_SCALAR: + src_param->swizzle = (token & VKD3D_SM4_SWIZZLE_MASK) >> VKD3D_SM4_SWIZZLE_SHIFT; + src_param->swizzle = (src_param->swizzle & 0x3) * 0x01010101; + break; - case VKD3D_SM4_SWIZZLE_VEC4: - src_param->swizzle = swizzle_from_sm4((token & VKD3D_SM4_SWIZZLE_MASK) >> VKD3D_SM4_SWIZZLE_SHIFT); - break; + case VKD3D_SM4_SWIZZLE_VEC4: + src_param->swizzle = swizzle_from_sm4((token & VKD3D_SM4_SWIZZLE_MASK) >> VKD3D_SM4_SWIZZLE_SHIFT); + break; - default: - FIXME("Unhandled swizzle type %#x.\n", swizzle_type); - break; + default: + FIXME("Unhandled swizzle type %#x.\n", swizzle_type); + break; + } + break; } + + default: + FIXME("Unhandled dimension %#x.\n", dimension); + break; } if (register_is_input_output(&src_param->reg) && !shader_sm4_validate_input_output_register(priv, @@ -2535,7 +2556,7 @@ bool hlsl_sm4_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_sem {"sv_groupid", false, VKD3D_SHADER_TYPE_COMPUTE, VKD3D_SM4_SWIZZLE_VEC4, VKD3D_SM5_RT_THREAD_GROUP_ID, false}, {"sv_groupthreadid", false, VKD3D_SHADER_TYPE_COMPUTE, VKD3D_SM4_SWIZZLE_VEC4, VKD3D_SM5_RT_LOCAL_THREAD_ID, false}, - {"sv_primitiveid", false, VKD3D_SHADER_TYPE_GEOMETRY, VKD3D_SM4_SWIZZLE_NONE, VKD3D_SM4_RT_PRIMID, false}, + {"sv_primitiveid", false, VKD3D_SHADER_TYPE_GEOMETRY, VKD3D_SM4_SWIZZLE_MASK4, VKD3D_SM4_RT_PRIMID, false}, /* Put sv_target in this table, instead of letting it fall through to * default varying allocation, so that the register index matches the @@ -3383,7 +3404,7 @@ static void sm4_register_from_deref(struct hlsl_ctx *ctx, struct sm4_register *r reg->type = VKD3D_SM4_RT_SAMPLER; reg->dim = VKD3D_SM4_DIMENSION_NONE; if (swizzle_type) - *swizzle_type = VKD3D_SM4_SWIZZLE_NONE; + *swizzle_type = VKD3D_SM4_SWIZZLE_MASK4; reg->idx[0] = var->regs[HLSL_REGSET_SAMPLERS].id; reg->idx[0] += hlsl_offset_from_deref_safe(ctx, deref); assert(deref->offset_regset == HLSL_REGSET_SAMPLERS); @@ -3515,7 +3536,7 @@ static void sm4_dst_from_node(struct sm4_dst_register *dst, const struct hlsl_ir static void sm4_src_from_constant_value(struct sm4_src_register *src, const struct hlsl_constant_value *value, unsigned int width, unsigned int map_writemask) { - src->swizzle_type = VKD3D_SM4_SWIZZLE_NONE; + src->swizzle_type = VKD3D_SM4_SWIZZLE_MASK4; src->reg.type = VKD3D_SM4_RT_IMMCONST; if (width == 1) { @@ -4061,7 +4082,7 @@ static void write_sm4_ld(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *buf index = hlsl_ir_constant(sample_index); memset(&instr.srcs[2], 0, sizeof(instr.srcs[2])); - instr.srcs[2].swizzle_type = VKD3D_SM4_SWIZZLE_NONE; + instr.srcs[2].swizzle_type = VKD3D_SM4_SWIZZLE_MASK4; reg->type = VKD3D_SM4_RT_IMMCONST; reg->dim = VKD3D_SM4_DIMENSION_SCALAR; reg->immconst_uint[0] = index->value.u[0].u; @@ -4181,7 +4202,7 @@ static void write_sm4_cast_from_bool(struct hlsl_ctx *ctx, instr.dst_count = 1; sm4_src_from_node(&instr.srcs[0], arg, instr.dsts[0].writemask); - instr.srcs[1].swizzle_type = VKD3D_SM4_SWIZZLE_NONE; + instr.srcs[1].swizzle_type = VKD3D_SM4_SWIZZLE_MASK4; instr.srcs[1].reg.type = VKD3D_SM4_RT_IMMCONST; instr.srcs[1].reg.dim = VKD3D_SM4_DIMENSION_SCALAR; instr.srcs[1].reg.immconst_uint[0] = mask; @@ -4782,7 +4803,7 @@ static void write_sm4_jump(struct hlsl_ctx *ctx, instr.opcode = VKD3D_SM4_OP_DISCARD | VKD3D_SM4_CONDITIONAL_NZ; memset(&instr.srcs[0], 0, sizeof(*instr.srcs)); - instr.srcs[0].swizzle_type = VKD3D_SM4_SWIZZLE_NONE; + instr.srcs[0].swizzle_type = VKD3D_SM4_SWIZZLE_MASK4; instr.src_count = 1; reg->type = VKD3D_SM4_RT_IMMCONST; reg->dim = VKD3D_SM4_DIMENSION_SCALAR; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225
From: Conor McCarthy <cmccarthy(a)codeweavers.com> For some resources the token contains a swizzle instead of a mask, resulting in a garbage value in write_mask. Read and validate the swizzle, which is relevant for future use of vector formats in image_format_for_image_read(). --- libs/vkd3d-shader/tpf.c | 69 +++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 24 deletions(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index a7ad5f1de..442ed6dba 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -1840,26 +1840,6 @@ static bool shader_sm4_read_param(struct vkd3d_shader_sm4_parser *priv, const ui return true; } -static bool shader_sm4_is_scalar_register(const struct vkd3d_shader_register *reg) -{ - switch (reg->type) - { - case VKD3DSPR_COVERAGE: - case VKD3DSPR_DEPTHOUT: - case VKD3DSPR_DEPTHOUTGE: - case VKD3DSPR_DEPTHOUTLE: - case VKD3DSPR_GSINSTID: - case VKD3DSPR_LOCALTHREADINDEX: - case VKD3DSPR_OUTPOINTID: - case VKD3DSPR_PRIMID: - case VKD3DSPR_SAMPLEMASK: - case VKD3DSPR_OUTSTENCILREF: - return true; - default: - return false; - } -} - static uint32_t swizzle_from_sm4(uint32_t s) { return vkd3d_shader_create_swizzle(s & 0x3, (s >> 2) & 0x3, (s >> 4) & 0x3, (s >> 6) & 0x3); @@ -2032,7 +2012,9 @@ static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, cons static bool shader_sm4_read_dst_param(struct vkd3d_shader_sm4_parser *priv, const uint32_t **ptr, const uint32_t *end, enum vkd3d_data_type data_type, struct vkd3d_shader_dst_param *dst_param) { + enum vkd3d_sm4_swizzle_type swizzle_type; enum vkd3d_shader_src_modifier modifier; + unsigned int dimension, swizzle; DWORD token; if (*ptr >= end) @@ -2054,12 +2036,51 @@ static bool shader_sm4_read_dst_param(struct vkd3d_shader_sm4_parser *priv, cons return false; } - dst_param->write_mask = (token & VKD3D_SM4_WRITEMASK_MASK) >> VKD3D_SM4_WRITEMASK_SHIFT; + switch ((dimension = (token & VKD3D_SM4_DIMENSION_MASK) >> VKD3D_SM4_DIMENSION_SHIFT)) + { + case VKD3D_SM4_DIMENSION_NONE: + dst_param->write_mask = 0; + break; + + case VKD3D_SM4_DIMENSION_SCALAR: + dst_param->write_mask = VKD3DSP_WRITEMASK_0; + break; + + case VKD3D_SM4_DIMENSION_VEC4: + swizzle_type = (token & VKD3D_SM4_SWIZZLE_TYPE_MASK) >> VKD3D_SM4_SWIZZLE_TYPE_SHIFT; + switch (swizzle_type) + { + case VKD3D_SM4_SWIZZLE_MASK4: + dst_param->write_mask = (token & VKD3D_SM4_WRITEMASK_MASK) >> VKD3D_SM4_WRITEMASK_SHIFT; + break; + + case VKD3D_SM4_SWIZZLE_VEC4: + swizzle = swizzle_from_sm4((token & VKD3D_SM4_SWIZZLE_MASK) >> VKD3D_SM4_SWIZZLE_SHIFT); + if (swizzle != VKD3D_SHADER_NO_SWIZZLE) + FIXME("Unhandled swizzle %#x.\n", swizzle); + dst_param->write_mask = VKD3DSP_WRITEMASK_ALL; + break; + + case VKD3D_SM4_SWIZZLE_SCALAR: + swizzle = (token & VKD3D_SM4_SWIZZLE_MASK) >> VKD3D_SM4_SWIZZLE_SHIFT; + FIXME("Making mask from component %#x.\n", swizzle); + dst_param->write_mask = VKD3DSP_WRITEMASK_0 << (swizzle & 3); + break; + + default: + FIXME("Unhandled swizzle type %#x.\n", swizzle_type); + break; + } + break; + + default: + FIXME("Unhandled dimension %#x.\n", dimension); + break; + } + if (data_type == VKD3D_DATA_DOUBLE) dst_param->write_mask = vkd3d_write_mask_64_from_32(dst_param->write_mask); - /* Scalar registers are declared with no write mask in shader bytecode. */ - if (!dst_param->write_mask && shader_sm4_is_scalar_register(&dst_param->reg)) - dst_param->write_mask = VKD3DSP_WRITEMASK_0; + dst_param->modifiers = 0; dst_param->shift = 0; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225
VKD3D_SM4_SWIZZLE_NONE is incorrect; the token contains a mask in this case.
Which is still no swizzle. :) I.e., I'd argue this is more of a matter of interpretation than necessarily being incorrect.
```diff + if (mask == VKD3DSP_WRITEMASK_0) + src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); + else if (!mask) + src_param->swizzle = 0; + else if (mask != VKD3DSP_WRITEMASK_ALL) + FIXME("Unhandled mask %#x.\n", mask); ```
Note that "0" above is effectively "VKD3D_SHADER_SWIZZLE(X, X, X, X)". That applies to the "VKD3D_SM4_DIMENSION_NONE" case as well. The most significant change in patch 1/2 seems replacing usage of shader_sm4_is_scalar_register() with checking for VKD3D_SM4_DIMENSION_SCALAR. The other change I see is changing some cases (e.g. VKD3D_SM4_DIMENSION_VEC4 + VKD3DSP_WRITEMASK_0) from VKD3D_SHADER_NO_SWIZZLE to VKD3D_SHADER_SWIZZLE(X, X, X, X). That's probably fine, both those might as well be two separate changes. Patch 2/2 is similar; it introduces usage of "dimension", and then tweaks a couple of cases. Separating those changes would probably make it more obvious what's going on. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225#note_36825
On Tue Jun 27 04:40:06 2023 +0000, Henri Verbeet wrote:
VKD3D_SM4_SWIZZLE_NONE is incorrect; the token contains a mask in this case. Which is still no swizzle. :) I.e., I'd argue this is more of a matter of interpretation than necessarily being incorrect. ```diff + if (mask == VKD3DSP_WRITEMASK_0) + src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); + else if (!mask) + src_param->swizzle = 0; + else if (mask != VKD3DSP_WRITEMASK_ALL) + FIXME("Unhandled mask %#x.\n", mask); ``` Note that "0" above is effectively "VKD3D_SHADER_SWIZZLE(X, X, X, X)". That applies to the "VKD3D_SM4_DIMENSION_NONE" case as well. The most significant change in patch 1/2 seems replacing usage of shader_sm4_is_scalar_register() with checking for VKD3D_SM4_DIMENSION_SCALAR. The other change I see is changing some cases (e.g. VKD3D_SM4_DIMENSION_VEC4 + VKD3DSP_WRITEMASK_0) from VKD3D_SHADER_NO_SWIZZLE to VKD3D_SHADER_SWIZZLE(X, X, X, X). That's probably fine, both those might as well be two separate changes. Patch 2/2 is similar; it introduces usage of "dimension", and then tweaks a couple of cases. Separating those changes would probably make it more obvious what's going on. I intended `swizzle = 0` to show the swizzle is unused, but this is probably better handled with comments.
At this point I'm not aware of these changes being necessary for any reason, but loading a swizzle when it could be a mask, and vice versa, raises the potential for edge cases which pass silently but which we would want to know about, e.g. in handling the mask when we only store a swizzle. I'll revise it if there's a consensus it should be taken care of. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225#note_36923
At this point I'm not aware of these changes being necessary for any reason, but loading a swizzle when it could be a mask, and vice versa, raises the potential for edge cases which pass silently but which we would want to know about, e.g. in handling the mask when we only store a swizzle.
Yeah, it probably makes sense to clean the parsing here up a little instead of assuming sources will have swizzles and destinations will have write masks. I wouldn't be surprised if certain combinations should simply be rejected either. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225#note_36958
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/tpf.c:
+ + switch (swizzle_type) + { + case VKD3D_SM4_SWIZZLE_MASK4: + mask = (token & VKD3D_SM4_WRITEMASK_MASK) >> VKD3D_SM4_WRITEMASK_SHIFT; + src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; - break; + if (mask == VKD3DSP_WRITEMASK_0) + src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); + else if (!mask) + src_param->swizzle = 0; + else if (mask != VKD3DSP_WRITEMASK_ALL) + FIXME("Unhandled mask %#x.\n", mask); + + if (!mask && (src_param->reg.type == VKD3DSPR_IMMCONST || src_param->reg.type == VKD3DSPR_IMMCONST64)) nitpick: Wouldn't it make sense for this conditional to be the first `else if` above?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225#note_37067
Nice! I didn't knew that all the information required to know whether to parse the swizzle or the writemask was embedded in the register data, I thought that whether to parse the register as a src or a dst depended on the instruction. I am looking at the bytes of the native compiler output with some complex shaders and this pattern checks out.  -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225#note_37068
This merge request was approved by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225
@fcasas in case you were not aware, this info is from: https://github.com/microsoft/DirectXShaderCompiler/blob/main/include/dxc/Sup... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/225#note_38272
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Francisco Casas (＠fcasas)
-
Henri Verbeet (＠hverbeet)