Skip to content
Snippets Groups Projects

vkd3d-shader/d3dbc: Make signature masks contiguous.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:etna into master
2 unresolved threads

Consider this HLSL pixel shader:

float4 main(float4 color : COLOR) : SV_Target
{
    return float4(color.x, 10.0f, 11.0f, color.w);
}

Currently the parser describes the input signature element corresponding to semantic COLOR as having mask .xw, which is sensible. However, the SPIR-V parser will interpret that as a mask starting at x and with popcount 2, and assuming it is contiguous it will implicitly act as if it were .xy. This is not correct, because the wrong component will be loaded from the vertex stage.

This MR seeks to fix this problem by making all signature masks contiguous, adding the missing bits if necessary. AFAICT that's already the case for all TPF and DXIL programs. But I'm not sure there are no corner cases in which adding mask bits can be a problem, so I'd welcome some feedback especially by @zfigura. It might be that the correct solution is to make the SPIR-V backend able to handle discontiguous masks.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • I forgot to mention that unfortunately it's not easy to write a test for this. In principle something like giomasce/vkd3d@671f164f would work, if we could have the pixel shader compiled by the native compiler. Our compiler loads the whole input register anyway, so the signature mask is .xyzw anyway. Instead with the native compiler this is generated:

      ps_3_0
      def c0, 1, 0, 10, 11
      dcl_color v0.xw
      mad oC0, v0.xxxw, c0.xyyx, c0.yzwy
    • The test was broken, it seems that giomasce/vkd3d@20832d48 is correct instead.

    • I think we should have the test, if for no other reason than that we want the HLSL compiler to do that kind of optimization eventually anyway.

    • Ok, I pushed the test with a comment explaining its current problem. It's a correct test anyway, even if currently less useful then we'd like it to be.

    • Interestingly, that test actually found a bug in SM<4 anyway, even if not the one this MR is directly concerned with. Since the position is never read by the pixel shader, it doesn't exist in its signature as reconstructed by vkd3d-shader, therefore the color register is matched with the position register written by the vertex shader. I'm not sure what's the right thing to do here, so I'm just leaving the test marked as todo(sm<4).

    • The right thing to do is actually use the varying map, which I thought I submitted patches for, but never did. I'll submit those soon.

    • Please register or sign in to reply
    • I think this works. It works for sm4 too, because while the used mask can be discontiguous, the actual mask won't be. I don't know if there's an sm6 concern, but I gather that the signatures are under our control there as well.

      I had to double check, but you're not allowed to have two dcl instructions with non-overlapping components of the same input register when authoring assembly either.

      Note: you can have arbitrary masks on vs_3_0 outputs too; the HLSL compiler even lets you do that. Theoretically you could have a shader that matches .xw with .xw. However, you can equally theoretically have a shader that matches .xw with .xyzw, and that kind of implies we need the VS to write dummy values anyway. Deciding when exactly to do that is probably nontrivial, but at least we have the varying map already to provide us with that information.

    • I think this works. It works for sm4 too, because while the used mask can be discontiguous, the actual mask won't be. I don't know if there's an sm6 concern, but I gather that the signatures are under our control there as well.

      IIRC DXIL directly expresses masks as a component index (called "column") and width, so they're necessarily contiguous.

    • Please register or sign in to reply
  • Elizabeth Figura approved this merge request

    approved this merge request

  • added 1 commit

    • 1306e4e4 - tests: Test discontiguous signature masks with SM<4 shaders.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • added 18 commits

    • 1306e4e4...bc351f4b - 15 commits from branch wine:master
    • 9893a337 - vkd3d-shader/d3dbc: Make signature masks contiguous.
    • 0361034c - vkd3d-shader/ir: Check that signature masks are contiguous.
    • dbbba78e - tests: Test discontiguous signature masks with SM<4 shaders.

    Compare with previous version

  • Giovanni Mascellani mentioned in merge request !1188 (merged)

    mentioned in merge request !1188 (merged)

  • Henri Verbeet approved this merge request

    approved this merge request

  • Henri Verbeet added 41 commits

    added 41 commits

    • dbbba78e...8943999b - 38 commits from branch wine:master
    • 083b87c7 - vkd3d-shader/d3dbc: Make signature masks contiguous.
    • 0e43dea4 - vkd3d-shader/ir: Check that signature masks are contiguous.
    • e411fcb6 - tests: Test discontiguous signature masks with SM<4 shaders.

    Compare with previous version

Please register or sign in to reply
Loading