[PATCH v4 0/4] MR136: vkd3d-shader/hlsl: Handle discard statement.
-- v4: tests: Remove rtv clears in d3d12 runner. tests: Add a simple test for "discard". vkd3d-shader/hlsl: Handle discard statement. vkd3d-shader/trace: Add separate id for discard. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136
From: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/dxbc.c | 2 +- libs/vkd3d-shader/spirv.c | 1 + libs/vkd3d-shader/trace.c | 3 ++- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c index f32af8ea..baf42f4a 100644 --- a/libs/vkd3d-shader/dxbc.c +++ b/libs/vkd3d-shader/dxbc.c @@ -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"}, diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 9504b955..76c0a391 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -7878,6 +7878,7 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c spirv_compiler_emit_retc(compiler, instruction); break; + case VKD3DSIH_DISCARD: case VKD3DSIH_TEXKILL: spirv_compiler_emit_kill(compiler, instruction); break; diff --git a/libs/vkd3d-shader/trace.c b/libs/vkd3d-shader/trace.c index d17a2819..9438bfac 100644 --- a/libs/vkd3d-shader/trace.c +++ b/libs/vkd3d-shader/trace.c @@ -109,6 +109,7 @@ static const char * const shader_opcode_names[] = [VKD3DSIH_DEQ ] = "deq", [VKD3DSIH_DFMA ] = "dfma", [VKD3DSIH_DGE ] = "dge", + [VKD3DSIH_DISCARD ] = "discard", [VKD3DSIH_DIV ] = "div", [VKD3DSIH_DLT ] = "dlt", [VKD3DSIH_DMAX ] = "dmax", @@ -1505,9 +1506,9 @@ static void shader_dump_instruction_flags(struct vkd3d_d3d_asm_compiler *compile { case VKD3DSIH_BREAKP: case VKD3DSIH_CONTINUEP: + case VKD3DSIH_DISCARD: case VKD3DSIH_IF: case VKD3DSIH_RETP: - case VKD3DSIH_TEXKILL: switch (ins->flags) { case VKD3D_SHADER_CONDITIONAL_OP_NZ: shader_addline(buffer, "_nz"); break; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 7e111c42..745a9874 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -223,6 +223,7 @@ enum vkd3d_shader_opcode VKD3DSIH_DEQ, VKD3DSIH_DFMA, VKD3DSIH_DGE, + VKD3DSIH_DISCARD, VKD3DSIH_DIV, VKD3DSIH_DLT, VKD3DSIH_DMAX, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 14 ++++++++++++++ libs/vkd3d-shader/hlsl_sm4.c | 16 ++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 3873a939..44ff2ea7 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3998,6 +3998,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <list> conditional_expr %type <list> declaration %type <list> declaration_statement +%type <list> discard_statement %type <list> equality_expr %type <list> expr %type <list> expr_optional @@ -5236,6 +5237,7 @@ statement: declaration_statement | expr_statement | compound_statement + | discard_statement | jump_statement | selection_statement | loop_statement @@ -5255,6 +5257,18 @@ jump_statement: YYABORT; } +discard_statement: + KW_DISCARD ';' + { + struct hlsl_ir_jump *discard; + + if (!($$ = make_empty_list(ctx))) + YYABORT; + if (!(discard = hlsl_new_jump(ctx, HLSL_IR_JUMP_DISCARD, @1))) + return false; + list_add_tail($$, &discard->node.entry); + } + selection_statement: KW_IF '(' expr ')' if_body { diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index 5bd4f2d6..85356e24 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -2144,6 +2144,22 @@ static void write_sm4_jump(struct hlsl_ctx *ctx, instr.opcode = VKD3D_SM4_OP_BREAK; break; + case HLSL_IR_JUMP_DISCARD: + { + struct sm4_register *reg = &instr.srcs[0].reg; + + instr.opcode = VKD3D_SM4_OP_DISCARD | VKD3D_SM4_CONDITIONAL_NZ; + + memset(&instr.srcs[0], 0, sizeof(*instr.srcs)); + instr.srcs[0].swizzle_type = VKD3D_SM4_SWIZZLE_NONE; + instr.src_count = 1; + reg->type = VKD3D_SM4_RT_IMMCONST; + reg->dim = VKD3D_SM4_DIMENSION_SCALAR; + reg->immconst_uint[0] = ~0u; + + break; + } + case HLSL_IR_JUMP_RETURN: vkd3d_unreachable(); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- Makefile.am | 1 + tests/hlsl-discard.shader_test | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 tests/hlsl-discard.shader_test diff --git a/Makefile.am b/Makefile.am index 48991982..42d90822 100644 --- a/Makefile.am +++ b/Makefile.am @@ -74,6 +74,7 @@ vkd3d_shader_tests = \ tests/hlsl-clamp.shader_test \ tests/hlsl-comma.shader_test \ tests/hlsl-cross.shader_test \ + tests/hlsl-discard.shader_test \ tests/hlsl-dot.shader_test \ tests/hlsl-duplicate-modifiers.shader_test \ tests/hlsl-for.shader_test \ diff --git a/tests/hlsl-discard.shader_test b/tests/hlsl-discard.shader_test new file mode 100644 index 00000000..d99c49c6 --- /dev/null +++ b/tests/hlsl-discard.shader_test @@ -0,0 +1,16 @@ +[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) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136
From: Nikolay Sivov <nsivov(a)codeweavers.com> --- tests/shader_runner_d3d12.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 87e5d130..3480498b 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -337,7 +337,6 @@ static bool d3d12_runner_draw(struct shader_runner *r, ID3D12CommandQueue *queue = test_context->queue; D3D12_INPUT_ELEMENT_DESC *input_element_descs; ID3D12Device *device = test_context->device; - static const float clear_color[4]; ID3D10Blob *vs_code, *ps_code; unsigned int uniform_index; unsigned int rtv_count = 0; @@ -424,8 +423,6 @@ static bool d3d12_runner_draw(struct shader_runner *r, { case RESOURCE_TYPE_RENDER_TARGET: rtvs[resource->r.slot] = get_cpu_rtv_handle(test_context, runner->rtv_heap, resource->r.slot); - ID3D12GraphicsCommandList_ClearRenderTargetView(command_list, - rtvs[resource->r.slot], clear_color, 0, NULL); rtv_count = max(rtv_count, resource->r.slot + 1); break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136
On Mon Apr 10 11:14:35 2023 +0000, Nikolay Sivov wrote:
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. 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_29625
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_29720
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... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_29774
First for consistency all runners should do the same thing. For clearing on every [test], it will probably need an explicit call in ops. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_29776
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_29889
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). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30055
On Fri Apr 14 08:59:27 2023 +0000, Giovanni Mascellani wrote:
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) ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30099
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30343
On Tue Apr 18 13:57:34 2023 +0000, Henri Verbeet wrote:
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) ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30371
Next rebase will need a small tweak: ```diff 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30600
On Thu Apr 20 04:30:07 2023 +0000, Ethan Lee wrote:
Next rebase will need a small tweak: ```diff 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 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: ```diff diff --git a/tests/hlsl-discard.shader_test b/tests/hlsl-discard.shader_test index d99c49c6..0b1c3bcc 100644 --- a/tests/hlsl-discard.shader_test +++ b/tests/hlsl-discard.shader_test @@ -3,14 +3,16 @@ 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 1 2 3 4 +draw quad +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! -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30614
On Thu Apr 20 04:33:16 2023 +0000, Ethan Lee wrote:
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 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 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! I think all of that makes sense, yeah.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30629
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 -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/136#note_30807
participants (6)
-
Ethan Lee (＠flibitijibibo) -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)