[PATCH 0/1] MR349: vkd3d-shader/spirv: Ensure that all instructions belong to a block.
This is done by creating dummy blocks, which do not have any incoming branch. SPIR-V specification doesn't allow instructions not belonging to any block. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> This is done by creating dummy blocks, which do not have any incoming branch. SPIR-V specification doesn't allow instructions not belonging to any block. --- libs/vkd3d-shader/spirv.c | 76 ++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 95f6914a..24d2ef7e 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2222,7 +2222,6 @@ struct vkd3d_control_flow_info VKD3D_BLOCK_LOOP, VKD3D_BLOCK_SWITCH, } current_block; - bool inside_block; }; struct vkd3d_push_constant_buffer_binding @@ -2274,6 +2273,7 @@ struct spirv_compiler unsigned int branch_id; unsigned int loop_id; unsigned int switch_id; + unsigned int skip_id; unsigned int control_flow_depth; struct vkd3d_control_flow_info *control_flow_info; size_t control_flow_info_size; @@ -7280,7 +7280,7 @@ static struct vkd3d_control_flow_info *spirv_compiler_find_innermost_breakable_c static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *compiler, const struct vkd3d_shader_instruction *instruction) { - uint32_t loop_header_block_id, loop_body_block_id, continue_block_id; + uint32_t loop_header_block_id, loop_body_block_id, continue_block_id, skip_block_id; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; const struct vkd3d_shader_src_param *src = instruction->src; uint32_t merge_block_id, val_id, condition_id, true_label; @@ -7309,7 +7309,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c cf_info->u.if_.id = compiler->branch_id; cf_info->u.if_.merge_block_id = merge_block_id; cf_info->u.if_.else_block_id = 0; - cf_info->inside_block = true; cf_info->current_block = VKD3D_BLOCK_IF; vkd3d_spirv_build_op_name(builder, merge_block_id, "branch%u_merge", compiler->branch_id); @@ -7321,8 +7320,7 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c assert(compiler->control_flow_depth); assert(cf_info->current_block == VKD3D_BLOCK_IF); - if (cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, cf_info->u.if_.merge_block_id); + vkd3d_spirv_build_op_branch(builder, cf_info->u.if_.merge_block_id); cf_info->u.if_.else_block_id = vkd3d_spirv_alloc_id(builder); vkd3d_spirv_as_op_branch_conditional(&builder->function_stream, @@ -7330,15 +7328,13 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c vkd3d_spirv_build_op_name(builder, cf_info->u.if_.else_block_id, "branch%u_false", cf_info->u.if_.id); vkd3d_spirv_build_op_label(builder, cf_info->u.if_.else_block_id); - cf_info->inside_block = true; break; case VKD3DSIH_ENDIF: assert(compiler->control_flow_depth); assert(cf_info->current_block == VKD3D_BLOCK_IF); - if (cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, cf_info->u.if_.merge_block_id); + vkd3d_spirv_build_op_branch(builder, cf_info->u.if_.merge_block_id); vkd3d_spirv_build_op_label(builder, cf_info->u.if_.merge_block_id); @@ -7365,7 +7361,6 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c cf_info->u.loop.continue_block_id = continue_block_id; cf_info->u.loop.merge_block_id = merge_block_id; cf_info->current_block = VKD3D_BLOCK_LOOP; - cf_info->inside_block = true; vkd3d_spirv_build_op_name(builder, loop_header_block_id, "loop%u_header", compiler->loop_id); vkd3d_spirv_build_op_name(builder, loop_body_block_id, "loop%u_body", compiler->loop_id); @@ -7378,10 +7373,7 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c assert(compiler->control_flow_depth); assert(cf_info->current_block == VKD3D_BLOCK_LOOP); - /* The loop block may have already been ended by an unconditional - * break instruction right before the end of the loop. */ - if (cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, cf_info->u.loop.continue_block_id); + vkd3d_spirv_build_op_branch(builder, cf_info->u.loop.continue_block_id); vkd3d_spirv_build_op_label(builder, cf_info->u.loop.continue_block_id); vkd3d_spirv_build_op_branch(builder, cf_info->u.loop.header_block_id); @@ -7395,6 +7387,7 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c return VKD3D_ERROR_OUT_OF_MEMORY; merge_block_id = vkd3d_spirv_alloc_id(builder); + skip_block_id = vkd3d_spirv_alloc_id(builder); assert(src->reg.data_type == VKD3D_DATA_INT); val_id = spirv_compiler_emit_load_src(compiler, src, VKD3DSP_WRITEMASK_0); @@ -7409,12 +7402,15 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c cf_info->u.switch_.case_blocks_size = 0; cf_info->u.switch_.case_block_count = 0; cf_info->u.switch_.default_block_id = 0; - cf_info->inside_block = false; cf_info->current_block = VKD3D_BLOCK_SWITCH; + vkd3d_spirv_build_op_label(builder, skip_block_id); + vkd3d_spirv_build_op_name(builder, merge_block_id, "switch%u_merge", compiler->switch_id); + vkd3d_spirv_build_op_name(builder, skip_block_id, "skip%u", compiler->skip_id); ++compiler->switch_id; + ++compiler->skip_id; if (!vkd3d_array_reserve((void **)&cf_info->u.switch_.case_blocks, &cf_info->u.switch_.case_blocks_size, 10, sizeof(*cf_info->u.switch_.case_blocks))) @@ -7425,7 +7421,10 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c case VKD3DSIH_ENDSWITCH: assert(compiler->control_flow_depth); assert(cf_info->current_block == VKD3D_BLOCK_SWITCH); - assert(!cf_info->inside_block); + + /* The current block will be ignored, but we still need to + * terminate it to generate valid SPIR-V. */ + vkd3d_spirv_build_op_return(builder); if (!cf_info->u.switch_.default_block_id) cf_info->u.switch_.default_block_id = cf_info->u.switch_.merge_block_id; @@ -7466,15 +7465,13 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c return VKD3D_ERROR_OUT_OF_MEMORY; label_id = vkd3d_spirv_alloc_id(builder); - if (cf_info->inside_block) /* fall-through */ - vkd3d_spirv_build_op_branch(builder, label_id); + vkd3d_spirv_build_op_branch(builder, label_id); cf_info->u.switch_.case_blocks[2 * cf_info->u.switch_.case_block_count + 0] = value; cf_info->u.switch_.case_blocks[2 * cf_info->u.switch_.case_block_count + 1] = label_id; ++cf_info->u.switch_.case_block_count; vkd3d_spirv_build_op_label(builder, label_id); - cf_info->inside_block = true; vkd3d_spirv_build_op_name(builder, label_id, "switch%u_case%u", cf_info->u.switch_.id, value); break; } @@ -7485,13 +7482,11 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c assert(!cf_info->u.switch_.default_block_id); cf_info->u.switch_.default_block_id = vkd3d_spirv_alloc_id(builder); - if (cf_info->inside_block) /* fall-through */ - vkd3d_spirv_build_op_branch(builder, cf_info->u.switch_.default_block_id); + vkd3d_spirv_build_op_branch(builder, cf_info->u.switch_.default_block_id); vkd3d_spirv_build_op_label(builder, cf_info->u.switch_.default_block_id); vkd3d_spirv_build_op_name(builder, cf_info->u.switch_.default_block_id, "switch%u_default", cf_info->u.switch_.id); - cf_info->inside_block = true; break; case VKD3DSIH_BREAK: @@ -7506,19 +7501,19 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c return VKD3D_ERROR_INVALID_SHADER; } + skip_block_id = vkd3d_spirv_alloc_id(builder); + if (breakable_cf_info->current_block == VKD3D_BLOCK_LOOP) - { vkd3d_spirv_build_op_branch(builder, breakable_cf_info->u.loop.merge_block_id); - } else if (breakable_cf_info->current_block == VKD3D_BLOCK_SWITCH) - { - /* The current case block may have already been ended by an - * unconditional continue instruction. */ - if (breakable_cf_info->inside_block) - vkd3d_spirv_build_op_branch(builder, breakable_cf_info->u.switch_.merge_block_id); - } + vkd3d_spirv_build_op_branch(builder, breakable_cf_info->u.switch_.merge_block_id); + + vkd3d_spirv_build_op_label(builder, skip_block_id); + + vkd3d_spirv_build_op_name(builder, skip_block_id, "skip%u", compiler->skip_id); + + ++compiler->skip_id; - cf_info->inside_block = false; break; } @@ -7552,9 +7547,16 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c return VKD3D_ERROR_INVALID_SHADER; } + skip_block_id = vkd3d_spirv_alloc_id(builder); + vkd3d_spirv_build_op_branch(builder, loop_cf_info->u.loop.continue_block_id); - cf_info->inside_block = false; + vkd3d_spirv_build_op_label(builder, skip_block_id); + + vkd3d_spirv_build_op_name(builder, skip_block_id, "skip%u", compiler->skip_id); + + ++compiler->skip_id; + break; } @@ -7578,9 +7580,19 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c spirv_compiler_emit_return(compiler, instruction); if (cf_info) - cf_info->inside_block = false; + { + skip_block_id = vkd3d_spirv_alloc_id(builder); + + vkd3d_spirv_build_op_label(builder, skip_block_id); + + vkd3d_spirv_build_op_name(builder, skip_block_id, "skip%u", compiler->skip_id); + + ++compiler->skip_id; + } else + { compiler->main_block_open = false; + } break; case VKD3DSIH_RETP: -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349
I think the backend should reject shaders with instructions outside blocks, except perhaps `nop`. A valid vsir shader should also contain no instructions outside blocks. Validating that in TPF is too complicated, but this touches on an issue which will arise in DXIL when branch and switch instructions are added. Using the same vsir instructions for both SM4/5 and SM6 shaders would require handling the SM4/5 structured control flow instructions in a normalisation stage, which would perform all validations. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45730
On Mon Sep 18 20:09:45 2023 +0000, Conor McCarthy wrote:
I think the backend should reject shaders with instructions outside blocks, except perhaps `nop`. A valid vsir shader should also contain no instructions outside blocks. Validating that in TPF is too complicated, but this touches on an issue which will arise in DXIL when branch and switch instructions are added. Using the same vsir instructions for both SM4/5 and SM6 shaders would require handling the SM4/5 structured control flow instructions in a normalisation stage, which would perform all validations. Hmmm, in my case the VSIR shader passed to the SPIR-V backend is valid and has no instructions outside a block. However, when dealing with unconditional control flow (e.g. a `return` or `break`) inside a loop, the SPIR-V generator outputs, say, `OpReturn`, which terminates the current block, but then goes on outputting any other dead code which appears after that `return` or `break` without opening another block. The generated SPIR-V is thus illegal.
I changed it so that after `OpReturn` another dummy SPIR-V block is began and the generated SPIR-V is valid. That's a problem with the SPIR-V code generation, not with VSIR. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45790
I changed it so that after `OpReturn` another dummy SPIR-V block is began and the generated SPIR-V is valid. That's a problem with the SPIR-V code generation, not with VSIR.
Shouldn't we just stop writing instructions in the current block after a jump? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45831
On Mon Sep 18 20:09:45 2023 +0000, Giovanni Mascellani wrote:
Hmmm, in my case the VSIR shader passed to the SPIR-V backend is valid and has no instructions outside a block. However, when dealing with unconditional control flow (e.g. a `return` or `break`) inside a loop, the SPIR-V generator outputs, say, `OpReturn`, which terminates the current block, but then goes on outputting any other dead code which appears after that `return` or `break` without opening another block. The generated SPIR-V is thus illegal. I changed it so that after `OpReturn` another dummy SPIR-V block is began and the generated SPIR-V is valid. That's a problem with the SPIR-V code generation, not with VSIR. Does this issue result from boilerplate code outside of the vsir instruction handlers? Certainly the backend should not emit outside a block if the vsir doesn't, so I wonder if there's a deeper issue here. The dummy block seems like a workaround, not a complete fix.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45849
On Tue Sep 19 09:00:26 2023 +0000, Zebediah Figura wrote:
I changed it so that after `OpReturn` another dummy SPIR-V block is began and the generated SPIR-V is valid. That's a problem with the SPIR-V code generation, not with VSIR. Shouldn't we just stop writing instructions in the current block after a jump? That's another approach. It's a bit more complicated because it requires more coordination with the rest of the code generator, which needs to know when to stop and resume emitting code, but at the same time still has to emit correct SPIR-V (it's not clear to me, say, if just stopping writing bytecode when you're not in a block is the right thing: as far as I know there could be other types of SPIR-V bytecode, such as annotations or I don't know what, that we still want to be generated). So my solution felt easier.
At the same time, my easier solution doesn't look too wrong: at least for the moment the SPIR-V generator is very crude, it receives VSIR instructions and directly emit the corresponding SPIR-V code without optimization. So if it receives dead code it is not inappropriate to emit other dead code: we just have to ensure that the SPIR-V code is valid, which is what this patch does. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45857
On Tue Sep 19 02:10:59 2023 +0000, Conor McCarthy wrote:
Does this issue result from boilerplate code outside of the vsir instruction handlers? Certainly the backend should not emit outside a block if the vsir doesn't, so I wonder if there's a deeper issue here. The dummy block seems like a workaround, not a complete fix. No, that comes directly from VSIR. Here is an HLSL example:
float4 main(float4 x : COLOR) : SV_TARGET
{
float4 ret = 0.0;
for (;;)
{
return ret;
ret = 1.0;
}
return ret;
}
This compiles (with our HLSL compiler) to this TPF code: ``` ps_4_0 dcl_output o0.xyzw dcl_temps 4 mov r0.x, l(0.00000000e+00) mov r1.xyzw, l(0.00000000e+00, 0.00000000e+00, 0.00000000e+00, 0.00000000e+00) loop mov r2.xyzw, r1.xyzw mov r3.xyzw, r2.xyzw mov r0.x, l(-nan) break mov r1.xyzw, l(1.00000000e+00, 1.00000000e+00, 1.00000000e+00, 1.00000000e+00) endloop mov r0.x, r0.x not r0.x, r0.x if_nz r0.x mov r0.xyzw, r1.xyzw mov r3.xyzw, r0.xyzw endif mov r0.xyzw, r3.xyzw mov o0.xyzw, r0.xyzw ret ``` This is valid TPF, but notice that there is some dead code after the `break`. After compiling to SPIR-V that code will still be present, but without a block: ``` ; SPIR-V ; Version: 1.0 ; Generator: Wine VKD3D Shader Compiler; 8 ; Bound: 44 ; Schema: 0 OpCapability Shader OpMemoryModel Logical GLSL450 OpEntryPoint Fragment %main "main" %o0 OpExecutionMode %main OriginUpperLeft OpName %main "main" OpName %r0 "r0" OpName %r1 "r1" OpName %r2 "r2" OpName %r3 "r3" OpName %o0 "o0" OpName %loop0_header "loop0_header" OpName %loop0_body "loop0_body" OpName %loop0_continue "loop0_continue" OpName %loop0_merge "loop0_merge" OpName %branch0_merge "branch0_merge" OpName %branch0_true "branch0_true" OpDecorate %o0 Location 0 %void = OpTypeVoid %3 = OpTypeFunction %void %float = OpTypeFloat 32 %v4float = OpTypeVector %float 4 %_ptr_Private_v4float = OpTypePointer Private %v4float %r0 = OpVariable %_ptr_Private_v4float Private %r1 = OpVariable %_ptr_Private_v4float Private %r2 = OpVariable %_ptr_Private_v4float Private %r3 = OpVariable %_ptr_Private_v4float Private %_ptr_Output_v4float = OpTypePointer Output %v4float %o0 = OpVariable %_ptr_Output_v4float Output %float_0 = OpConstant %float 0 %_ptr_Private_float = OpTypePointer Private %float %uint = OpTypeInt 32 0 %uint_0 = OpConstant %uint 0 %19 = OpConstantComposite %v4float %float_0 %float_0 %float_0 %float_0 %float_n0x1_fffffep_128 = OpConstant %float -0x1.fffffep+128 %float_1 = OpConstant %float 1 %27 = OpConstantComposite %v4float %float_1 %float_1 %float_1 %float_1 %bool = OpTypeBool %main = OpFunction %void None %3 %4 = OpLabel %18 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 OpStore %18 %float_0 OpStore %r1 %19 OpBranch %loop0_header %loop0_header = OpLabel OpLoopMerge %loop0_merge %loop0_continue None OpBranch %loop0_body %loop0_body = OpLabel OpCopyMemory %r2 %r1 OpCopyMemory %r3 %r2 %25 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 OpStore %25 %float_n0x1_fffffep_128 OpBranch %loop0_merge OpStore %r1 %27 %loop0_continue = OpLabel OpBranch %loop0_header %loop0_merge = OpLabel %28 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 %29 = OpLoad %float %28 %30 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 OpStore %30 %29 %31 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 %32 = OpLoad %float %31 %33 = OpBitcast %uint %32 %34 = OpNot %uint %33 %35 = OpBitcast %float %34 %36 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 OpStore %36 %35 %37 = OpInBoundsAccessChain %_ptr_Private_float %r0 %uint_0 %38 = OpLoad %float %37 %39 = OpBitcast %uint %38 %41 = OpINotEqual %bool %39 %uint_0 OpSelectionMerge %branch0_merge None OpBranchConditional %41 %branch0_true %branch0_merge %branch0_true = OpLabel OpCopyMemory %r0 %r1 OpCopyMemory %r3 %r0 OpBranch %branch0_merge %branch0_merge = OpLabel OpCopyMemory %r0 %r3 OpCopyMemory %o0 %r0 OpReturn OpFunctionEnd ``` This is not allowed by SPIR-V, which is what my MR is trying to fix: ``` error: line 53: Store must appear in a block OpStore %r1 %27 ``` However I just noticed that even with this MR there is still a validation problem: ``` error: line 56: Block '26[%skip0]' branches to the loop continue target '22[%loop0_continue]', but is not contained in the associated loop construct '20[%loop0_header]' OpBranch %loop0_continue ``` I thought that skipped blocks, being unreachable, were exempted from structure rules, but maybe I was mistaken. I'll check it better. Ignoring this last hiccup, that should explain why the MR is needed, even if with some additional tweak. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45858
I thought that skipped blocks, being unreachable, were exempted from structure rules
`OpUnreachable` instead of `OpBranch` should fix that. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45859
That's another approach. It's a bit more complicated because it requires more coordination with the rest of the code generator, which needs to know when to stop and resume emitting code,
Could we prune it from the IR before starting SPIR-V generation? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/349#note_45875
participants (5)
-
Conor McCarthy (＠cmccarthy) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)