Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Handle discard statement.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:hlsl_discard into master
6 unresolved threads

Merge request reports

Merge request pipeline #9961 skipped

Merge request pipeline skipped for 4fe4784e

Approved by

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Apr 26, 2023 8:59pm UTC)

Merge details

  • Changes merged into master with 4fe4784e.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Doesn't seem wrong, but as usual I think that some tests would be appropriate.

  • Nikolay Sivov added 1 commit

    added 1 commit

    • 5b78e9bd - tests: Add a simple test for "discard".

    Compare with previous version

  • Author Developer

    I pushed a test. As it happens d3d12 results stand up - texture seems to be cleared, on previous versions old content is retained. That's why I used 0 for test content.

    • @@ -633,7 +633,7 @@ static const struct vkd3d_sm4_opcode_info opcode_table[] =
           {VKD3D_SM4_OP_DEFAULT,                          VKD3DSIH_DEFAULT,                          "",     ""},
           {VKD3D_SM4_OP_DERIV_RTX,                        VKD3DSIH_DSX,                              "f",    "f"},
           {VKD3D_SM4_OP_DERIV_RTY,                        VKD3DSIH_DSY,                              "f",    "f"},
      -    {VKD3D_SM4_OP_DISCARD,                          VKD3DSIH_TEXKILL,                          "",     "u",
      +    {VKD3D_SM4_OP_DISCARD,                          VKD3DSIH_DISCARD,                          "",     "u",
                   shader_sm4_read_conditional_op},
           {VKD3D_SM4_OP_DIV,                              VKD3DSIH_DIV,                              "f",    "ff"},
           {VKD3D_SM4_OP_DP2,                              VKD3DSIH_DP2,                              "f",    "ff"},

      This breaks "discard" handling in the SPIR-V backend.

    • Author Developer

      I see what you mean, and have two questions now. Do we need both texkill -> SpvOpKill and discard -> SpvOpKill? For example, do we convert sm1 -> spirv directly? And second questions, what happens to texkill/discard arguments when translating to spirv?

    • Please register or sign in to reply
  • I see what you mean, and have two questions now. Do we need both texkill -> SpvOpKill and discard -> SpvOpKill? For example, do we convert sm1 -> spirv directly? And second questions, what happens to texkill/discard arguments when translating to spirv?

    We don't currently have d3dbc->spirv translation upstream, but @zfigura is working on adding that. I imagine that eventually we'll introduce a transformation pass to replace TEXKILL with an equivalent DISCARD, so that backends only need to handle DISCARD.

    As for the differences between (tex)kill and discard, shader_glsl_texkill() in wined3d is probably a decent reference. Note that the behaviour is slightly different between shader model 1 and shader model 2+ as well. There's also a slight difference in the allowed source registers between ps 1.4 and earlier versions, though that mostly only matters when generating bytecode.

    Note that the HLSL clip() intrinsic also exists.

  • Nikolay Sivov added 15 commits

    added 15 commits

    • 5b78e9bd...8186b752 - 12 commits from branch wine:master
    • 669757f3 - vkd3d-shader/trace: Add separate id for discard.
    • 7440ff61 - vkd3d-shader/hlsl: Handle discard statement.
    • 0862afe9 - tests: Add a simple test for "discard".

    Compare with previous version

  • We might need to throw away HLSL statements in the same block after a discard. In theory it wouldn't matter, but it does for derivatives. I have some half-completed tests I need to find and finish.

    • +[pixel shader]
      +uniform float4 x;
      +
      +float4 main() : sv_target
      +{
      +    if (x.x == 0.0f) discard;
      +    return float4(1, 2, 3, 4);
      +}
      +
      +[test]
      +uniform 0 float4 1 0 0 0
      +draw quad
      +probe all rgba (1, 2, 3, 4)
      +uniform 0 float4 0 0 0 0
      +draw quad
      +probe all rgba (1, 2, 3, 4)

      It's a bit unfortunate that simply ignoring "discard" would result in the test passing. It would seem helpful to either add support for clears to the shader runner (e.g., "clear (0, 1, 0, 0)") and use those here, or to pass the output colour as a uniform to the shader above and then use different colours for the two draws.

    • Author Developer

      I see now where confusion came from and why results differ in d3d12 - we have an explicit ClearRenderTargetView() with zero color. That's why with active discard previous contents are not preserved.

    • Author Developer

      Adding clear() command won't help as is, because d3d12 runner will clear unconditionally. For now I removed that unconditional clear, please take a look.

      Another option is to clear once, if it's really necessary. For example right on [test] section. That will have to touch all runners, but at the same time, d3d9/d3d11 ones do not clear already and somehow it's not a problem.

    • Please register or sign in to reply
  • Nikolay Sivov added 35 commits

    added 35 commits

    • 0862afe9...94ff9ba5 - 31 commits from branch wine:master
    • 2155ed94 - vkd3d-shader/trace: Add separate id for discard.
    • 476cc929 - vkd3d-shader/hlsl: Handle discard statement.
    • 189776ec - tests: Add a simple test for "discard".
    • 5f538bf3 - tests: Remove rtv clears in d3d12 runner.

    Compare with previous version

    • Adding clear() command won't help as is, because d3d12 runner will clear unconditionally. For now I removed that unconditional clear, please take a look.

      Another option is to clear once, if it's really necessary. For example right on [test] section. That will have to touch all runners, but at the same time, d3d9/d3d11 ones do not clear already and somehow it's not a problem.

      Oh, right. I don't think we want to clear before every draw, no. There may be an argument for clearing at the start of each test, although I think that in that case we'd like to clear with something other than zeroes.

    • Oh, right. I don't think we want to clear before every draw, no. There may be an argument for clearing at the start of each test, although I think that in that case we'd like to clear with something other than zeroes.

      Agreed. I wouldn't even bother clearing with something different than zero. Just clear with zero, and if the tests needs something else they can clear explicitly (or render with a trivial pixel shader, if we don't want a clear command).

    • Aside from the clearing issue, would a test like this make it more obvious that the second draw's uniform data went unused?

      [pixel shader]
      uniform float4 x;
      
      float4 main() : sv_target
      {
          if (x.x == 0.0f) discard;
          return x;
      }
      
      [test]
      uniform 0 float4 1 2 3 4
      draw quad
      probe all rgba (1, 2, 3, 4)
      uniform 0 float4 0 0 0 0
      draw quad
      probe all rgba (1, 2, 3, 4)
    • Please register or sign in to reply
  • Oh, right. I don't think we want to clear before every draw, no. There may be an argument for clearing at the start of each test, although I think that in that case we'd like to clear with something other than zeroes.

    Clearing at the start seems like a good idea. Clearing before every draw will get in the way of things like this, but it's also a bit unfortunate that we currently have some tests that draw the same pixel values multiple times...

  • Author Developer

    First for consistency all runners should do the same thing. For clearing on every [test], it will probably need an explicit call in ops.

  • Clearing at the start seems like a good idea. Clearing before every draw will get in the way of things like this, but it's also a bit unfortunate that we currently have some tests that draw the same pixel values multiple times...

    Yes, and an explicit "clear" statement may be helpful for some of those cases. Still, like here, in a lot of cases the test in question could simply avoid doing that.

    • Aside from the clearing issue, would a test like this make it more obvious that the second draw's uniform data went unused?

      Right, something along those lines is what I was trying to suggest earlier. I'd still argue all zeroes isn't ideal as a test value though.

    • I suppose we could change the values a bit...

      [pixel shader]
      uniform float4 x;
      
      float4 main() : sv_target
      {
          if (x.x == 9.0f) discard;
          return x;
      }
      
      [test]
      uniform 0 float4 1 2 3 4
      draw quad
      probe all rgba (1, 2, 3, 4)
      uniform 0 float4 9 8 7 6
      draw quad
      probe all rgba (1, 2, 3, 4)
    • Please register or sign in to reply
    • Next rebase will need a small tweak:

      diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
      index f9896ecb..ed248cf8 100644
      --- a/libs/vkd3d-shader/hlsl.y
      +++ b/libs/vkd3d-shader/hlsl.y
      @@ -5299,7 +5299,7 @@ discard_statement:
       
                   if (!($$ = make_empty_list(ctx)))
                       YYABORT;
      -            if (!(discard = hlsl_new_jump(ctx, HLSL_IR_JUMP_DISCARD, @1)))
      +            if (!(discard = hlsl_new_jump(ctx, HLSL_IR_JUMP_DISCARD, &@1)))
                       return false;
                   list_add_tail($$, &discard->node.entry);
               }

      This patchset rebased against main still works with my own local tests (FEZ, TMNT), however I couldn't get the unit test to pass locally; even the "9 8 7 6" test I posted earlier doesn't seem to work and I'm not 100% sure why... even when copying the any() test that MojoShader uses the probe returns the second float4 value, but replacing the discard statement with a return statement does work, so it's definitely something involving the discard statement specifically.

    • Thought about this a little bit more and realized that if FNA was working and vkd3d wasn't, this likely meant that DXBC was fine and SPIR-V was not - I skimmed through spirv.c and, remembering that TEXKILL and DISCARD got separated, I tried to match all the instances of both and realized that there was a missing case! I made this change and now the "9 8 7 6" test mostly works:

      diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c
      index b49f25c7..89346c1a 100644
      --- a/libs/vkd3d-shader/spirv.c
      +++ b/libs/vkd3d-shader/spirv.c
      @@ -9574,6 +9574,7 @@ static int spirv_compiler_handle_instruction(struct spirv_compiler *compiler,
               case VKD3DSIH_RET:
               case VKD3DSIH_RETP:
               case VKD3DSIH_SWITCH:
      +        case VKD3DSIH_DISCARD:
               case VKD3DSIH_TEXKILL:
                   ret = spirv_compiler_emit_control_flow_instruction(compiler, instruction);
                   break;

      The test did need some tweaking, as did the Vulkan runner; I assume this has more to do with the aforementioned clearing issues (EDIT: Had some extra lines in the test diff, woops):

      diff --git a/tests/hlsl-discard.shader_test b/tests/hlsl-discard.shader_test
      index d99c49c6..f543f877 100644
      --- a/tests/hlsl-discard.shader_test
      +++ b/tests/hlsl-discard.shader_test
      @@ -3,14 +3,14 @@ uniform float4 x;
       
       float4 main() : sv_target
       {
      -    if (x.x == 0.0f) discard;
      -    return float4(1, 2, 3, 4);
      +    if (x.x == 9.0f) discard;
      +    return x;
       }
       
       [test]
      -uniform 0 float4 1 0 0 0
      +uniform 0 float4 1 2 3 4
       draw quad
       probe all rgba (1, 2, 3, 4)
      -uniform 0 float4 0 0 0 0
      +uniform 0 float4 9 8 7 6
       draw quad
       probe all rgba (1, 2, 3, 4)
      diff --git a/tests/shader_runner_vulkan.c b/tests/shader_runner_vulkan.c
      index aca17a1a..8aff9c69 100644
      --- a/tests/shader_runner_vulkan.c
      +++ b/tests/shader_runner_vulkan.c
      @@ -966,8 +966,6 @@ static bool vulkan_runner_draw(struct shader_runner *r,
           clear_rect.baseArrayLayer = 0;
           clear_rect.layerCount = 1;
       
      -    VK_CALL(vkCmdClearAttachments(cmd_buffer, 1, &clear_attachment, 1, &clear_rect));
      -
           VK_CALL(vkCmdBindPipeline(cmd_buffer, VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline));
       
           bind_resources(runner, VK_PIPELINE_BIND_POINT_GRAPHICS, set_layout, pipeline_layout);

      This gets all tests to pass on my lab boxes!

      Edited by Ethan Lee
    • I think all of that makes sense, yeah.

    • Please register or sign in to reply
  • Nikolay Sivov added 73 commits

    added 73 commits

    • 5f538bf3...b46df551 - 68 commits from branch wine:master
    • 19208bde - vkd3d-shader/trace: Add separate id for discard.
    • e54b0500 - vkd3d-shader/hlsl: Handle discard statement.
    • eb9678ee - tests: Remove rtv clears in d3d12 runner.
    • 1099ef4c - tests: Remove rtv clears in Vulkan runner.
    • dd92c0e3 - tests: Add a simple test for "discard".

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 9 commits

    • dd92c0e3...06cc2e1a - 4 commits from branch wine:master
    • 6da7d16d - vkd3d-shader/trace: Add separate id for discard.
    • 59c63ec5 - vkd3d-shader/hlsl: Handle discard statement.
    • c74d148c - tests: Remove rtv clears in d3d12 runner.
    • e541e715 - tests: Remove rtv clears in Vulkan runner.
    • 4fe4784e - tests: Add a simple test for "discard".

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading