Skip to content
Snippets Groups Projects

vkd3d-shader/tpf: Read complete swizzle/mask info from parameter tokens.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:tpf_component into master

Merge request reports

Merge request pipeline #14175 skipped

Merge request pipeline skipped for 61841e94

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Aug 28, 2023 8:16pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Conor McCarthy added 1 commit

    added 1 commit

    • d5399053 - vkd3d-shader/tpf: Read complete swizzle/mask info for dst params.

    Compare with previous version

  • I know we briefly talked about this change yesterday, but the commit messages could do with some elaboration on why we're making this change.

  • Conor McCarthy added 2 commits

    added 2 commits

    • 4519a09a - vkd3d-shader/tpf: Read complete swizzle/mask info for src params.
    • 3db74964 - vkd3d-shader/tpf: Read complete swizzle/mask info for dst params.

    Compare with previous version

    • 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.

    • Author Developer

      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.

    • Please register or sign in to reply
  • 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.

  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 4519a09a
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))
  • 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.

    diagram.drawio

    Edited by Francisco Casas
  • Francisco Casas approved this merge request

    approved this merge request

  • Francisco Casas mentioned in merge request !269 (closed)

    mentioned in merge request !269 (closed)

  • Conor McCarthy added 2 commits

    added 2 commits

    • 9e2e7f6b - vkd3d-shader/tpf: Read complete swizzle/mask info for src params.
    • 89f059a8 - vkd3d-shader/tpf: Read complete swizzle/mask info for dst params.

    Compare with previous version

  • Conor McCarthy added 103 commits

    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.

    Compare with previous version

  • Francisco Casas mentioned in merge request !281 (merged)

    mentioned in merge request !281 (merged)

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • 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.

  • Conor McCarthy added 5 commits

    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.

    Compare with previous version

  • Conor McCarthy added 92 commits

    added 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.

    Compare with previous version

  • Conor McCarthy added 2 commits

    added 2 commits

    • f6d2df18 - vkd3d-shader/tpf: Handle the dimension bitfield in dst param tokens.
    • faf2b7da - vkd3d-shader/tpf: Handle the swizzle type bitfield in dst param tokens.

    Compare with previous version

  • Conor McCarthy added 3 commits

    added 3 commits

    • 6c86dd8f - vkd3d-shader/tpf: Use the default vec4 swizzle if a src param contains a mask.
    • ba4f3757 - vkd3d-shader/tpf: Handle the dimension bitfield in dst param tokens.
    • 5333cca1 - vkd3d-shader/tpf: Handle the swizzle type bitfield in dst param tokens.

    Compare with previous version

  • 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?

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading