vkd3d-shader/hlsl: Handle discard statement.
Merge request reports
Activity
@@ -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.
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.
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".
Toggle commit list-
5b78e9bd...8186b752 - 12 commits from branch
+[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.
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.
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.
Toggle commit list-
0862afe9...94ff9ba5 - 31 commits from branch
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)
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...
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.
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
Had a bit of extra time tonight, so I rebased this patchset against the latest and included my fixes:
https://gitlab.winehq.org/flibitijibibo/vkd3d/-/commits/discard-rebase/
Full list of changes:
- Added DISCARD case for spirv_compiler_handle_instruction, fixes the discard instruction being thrown out when generating SPIR-V
- hlsl.y discard parsing needed hlsl_ir_jump* to be updated to hlsl_ir_node*
- Added a commit to remove the clear call from the Vulkan runner
- The test is now the "9 8 7 6" test
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".
Toggle commit list-
5f538bf3...b46df551 - 68 commits from branch
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".
Toggle commit list-
dd92c0e3...06cc2e1a - 4 commits from branch