vkd3d-shader/tpf: Read complete swizzle/mask info from parameter tokens.
Merge request reports
Activity
added 1 commit
- d5399053 - vkd3d-shader/tpf: Read complete swizzle/mask info for dst params.
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.
+ 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.
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.
1986 1987 switch (swizzle_type) 1988 { 1989 case VKD3D_SM4_SWIZZLE_MASK4: 1990 mask = (token & VKD3D_SM4_WRITEMASK_MASK) >> VKD3D_SM4_WRITEMASK_SHIFT; 1991 1986 1992 src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; 1987 break; 1993 if (mask == VKD3DSP_WRITEMASK_0) 1994 src_param->swizzle = VKD3D_SHADER_SWIZZLE(X, X, X, X); 1995 else if (!mask) 1996 src_param->swizzle = 0; 1997 else if (mask != VKD3DSP_WRITEMASK_ALL) 1998 FIXME("Unhandled mask %#x.\n", mask); 1999 2000 if (!mask && (src_param->reg.type == VKD3DSPR_IMMCONST || src_param->reg.type == VKD3DSPR_IMMCONST64)) changed this line in version 4 of the diff
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.
Edited by Francisco Casasmentioned in merge request !269 (closed)
@fcasas in case you were not aware, this info is from: https://github.com/microsoft/DirectXShaderCompiler/blob/main/include/dxc/Support/d3d12TokenizedProgramFormat.hpp
added 103 commits
-
89f059a8...2a3413e0 - 101 commits from branch
wine:master
- ff8d9df2 - vkd3d-shader/tpf: Read complete swizzle/mask info for src params.
- 643374c8 - vkd3d-shader/tpf: Read complete swizzle/mask info for dst params.
-
89f059a8...2a3413e0 - 101 commits from branch
mentioned in merge request !281 (merged)
It may have gotten lost a bit in the discussion, but to be clear, I think this these patches should be split, specifically in parts that introduce parsing the dimension in shader_sm4_read_src_param() and shader_sm4_read_dst_param(), and in parts that actually change behaviour.
I'm not entirely convinced about the benefits of renaming VKD3D_SM4_SWIZZLE_NONE to VKD3D_SM4_SWIZZLE_MASK4, but in any case it seems largely orthogonal to the rest of this MR, so the expedient thing to do may be to split that off into its own MR and argue about its merits there.
added 5 commits
- 0ca2aeff - vkd3d-shader/tpf: Handle the dimension bitfield in src param tokens.
- 95a336c6 - vkd3d-shader/tpf: Use the default vec4 swizzle if a src param contains a mask.
- f70bf831 - vkd3d-shader/tpf: Handle the dimension bitfield in dst param tokens.
- d2035a2c - vkd3d-shader/tpf: Handle the swizzle type bitfield in dst param tokens.
- 8bd3fb1c - vkd3d-shader/tpf: Remove the scalar register workaround for dst param write masks.
Toggle commit listadded 92 commits
-
8bd3fb1c...f649db23 - 88 commits from branch
wine:master
- aadedcf6 - vkd3d-shader/tpf: Handle the dimension bitfield in src param tokens.
- 24e6f879 - vkd3d-shader/tpf: Use the default vec4 swizzle if a src param contains a mask.
- 77f096d2 - vkd3d-shader/tpf: Handle the dimension bitfield in dst param tokens.
- cb71fb1e - vkd3d-shader/tpf: Handle the swizzle type bitfield in dst param tokens.
Toggle commit list-
8bd3fb1c...f649db23 - 88 commits from branch
This seems largely fine like this, although I do wish it used vkd3d_shader_parser_error()/vkd3d_shader_parser_warning() instead of FIXME in more places.
@@ -1986,10 +1986,17 @@ static bool shader_sm4_read_src_param(struct vkd3d_shader_sm4_parser *priv, cons switch (swizzle_type) { 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 - src_param->swizzle = VKD3D_SHADER_NO_SWIZZLE; + assert(!shader_sm4_is_scalar_register(&src_param->reg));
What allows us to make that assertion?