Skip to content
Snippets Groups Projects

vkd3d-shader/ir: Remove dead code during normalisation.

Merged Giovanni Mascellani requested to merge giomasce/vkd3d:vesuvio into master
3 unresolved threads

Merge request reports

Merge request pipeline #15581 skipped

Merge request pipeline skipped for d9c8b49e

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Sep 25, 2023 8:28pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • 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.

    • 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.

    • I thought that skipped blocks, being unreachable, were exempted from structure rules

      OpUnreachable instead of OpBranch should fix that.

    • Please register or sign in to reply
    • 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.

    • Please register or sign in to reply
  • 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?

  • added 1 commit

    • 30424d38 - vkd3d-shader/ir: Remove dead code during normalisation.

    Compare with previous version

  • As suggested by Henri, the latest revision doesn't touch the SPIR-V generator, but removes dead code (or, rather, sets it to NOP, but that's enough) as a normalisation pass. Now my (admittedly simple) tests pass SPIR-V validation and the two relevant shader runner tests pass on macOS.

  • Giovanni Mascellani changed title from vkd3d-shader/spirv: Ensure that all instructions belong to a block. to vkd3d-shader/ir: Remove dead code during normalisation.

    changed title from vkd3d-shader/spirv: Ensure that all instructions belong to a block. to vkd3d-shader/ir: Remove dead code during normalisation.

  • Giovanni Mascellani changed the description

    changed the description

  • added 1 commit

    • d4ceaa38 - vkd3d-shader/ir: Remove dead code during normalisation.

    Compare with previous version

  • I fixed some cases not being handled properly (or at all). Now tests should pass. So they do locally, at least.

    • In spirv_compiler_emit_control_flow_instruction(), break and continue set the depth not to zero but to the innermost breakable construct. Setting depth to zero in this patch seems to work, but a comment explaining exactly why would be helpful.

    • I added a few comments, hopefully they make the logic clearer.

    • Please register or sign in to reply
  • added 1 commit

    • b9c79f40 - vkd3d-shader/ir: Remove dead code during normalisation.

    Compare with previous version

  • Conor McCarthy approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 64 commits

    added 64 commits

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading