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: ```c 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.