[AMDGPU] Move code sinking before structurizer

[piotr]

Summary

Moving code sinking pass before structurizer creates more sinking
opportunities.

The extra flow edges introduced by the structurizer can have adverse
effects on sinking, because the sinking pass prefers moving instructions
to blocks with unique predecessors and the structurizer destroys that
property in some cases.

A notable example is moving high-latency image instructions across kills.

[piotr]

Added test.

[arsenm]

I don't remember why we even run this extra sink run. The regular middle end optimizer runs it. I think this helped one edge case at one point. Do any tests regress if you just remove it entirely?

[piotr]

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

[arsenm]

In D101115#2711462, @piotr wrote:

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

I'm not worried about legacy, but whether it's redundant since it should have already run at this point

llvm/test/CodeGen/AMDGPU/multilevel-break.ll

198

Did this drop the memory operand or something? This looks like a regression

llvm/test/CodeGen/AMDGPU/sink-image-sample.ll

3 ↗

(On Diff #339799)

Maybe add a comment explaining what this is for?

[arsenm]

llvm/test/CodeGen/AMDGPU/multilevel-break.ll

198

Oh, these loads are already volatile and should have had glc set. Maybe this test was last regenerated before glc was emitted for volatile loads?

[piotr]

In D101115#2711964, @arsenm wrote:

In D101115#2711462, @piotr wrote:

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

I'm not worried about legacy, but whether it's redundant since it should have already run at this point

Some sinking is done as part of other passes, but I do not think this pass is set up to be run at any other point in our pass list.

[piotr]

llvm/test/CodeGen/AMDGPU/multilevel-break.ll

198

Good spot - I get the glc generated even without my patch. I will pre-commit those glc changes separately so they do not pop up here.

[piotr]

Rebased, added comment in test.

[piotr]

In D101115#2711972, @piotr wrote:

In D101115#2711964, @arsenm wrote:

In D101115#2711462, @piotr wrote:

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

I'm not worried about legacy, but whether it's redundant since it should have already run at this point

Some sinking is done as part of other passes, but I do not think this pass is set up to be run at any other point in our pass list.

@foad helped me understand what you mean (thanks). Yes, this pass should be part of the opt pipeline, but currently we rely on it being run in the codegen pipeline. I do not know the history of that, but Mesa also needs it here.

[arsenm]

In D101115#2712423, @piotr wrote:

In D101115#2711972, @piotr wrote:

In D101115#2711964, @arsenm wrote:

In D101115#2711462, @piotr wrote:

I know it is a legacy pass, but I am convinced of its usefulness in our flow - both in real-world content and lit testing (e.g., no_skip_no_successors in skip-if-dead.ll).

I'm not worried about legacy, but whether it's redundant since it should have already run at this point

Some sinking is done as part of other passes, but I do not think this pass is set up to be run at any other point in our pass list.

@foad helped me understand what you mean (thanks). Yes, this pass should be part of the opt pipeline, but currently we rely on it being run in the codegen pipeline. I do not know the history of that, but Mesa also needs it here.

The backend isn't responsible for adding all the general optimization passes, only cases where we might want them to cleanup other lowering or late optimizations. I don't remember why this ended up here but moving it is fine

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

bool GCNPassConfig::addPreISel() {
   addPass(createAMDGPULateCodeGenPreparePass());
   if (EnableAtomicOptimizations) {
     addPass(createAMDGPUAtomicOptimizerPass());
   }
 
   // FIXME: We need to run a pass to propagate the attributes when calls are
   // supported.
 
+  addPass(createSinkingPass());
   // Merge divergent exit nodes. StructurizeCFG won't recognize the multi-exit
   // regions formed by them.
   addPass(&AMDGPUUnifyDivergentExitNodesID);
   if (!LateCFGStructurize) {
     if (EnableStructurizerWorkarounds) {
       addPass(createFixIrreduciblePass());
       addPass(createUnifyLoopExitsPass());
     }
     addPass(createStructurizeCFGPass(false)); // true -> SkipUniformRegions
   }
-  addPass(createSinkingPass());
   addPass(createAMDGPUAnnotateUniformValues());
   if (!LateCFGStructurize) {
     addPass(createSIAnnotateControlFlowPass());
   }
   addPass(createLCSSAPass());
 
   return false;
 }

llvm/test/CodeGen/AMDGPU/loop_exit_with_xor.ll

 ; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx803 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
 ; Where the mask of lanes wanting to exit the loop on this iteration is not
 ; obviously already masked by exec (in this case, the xor with -1 inserted by
 ; control flow annotation), then lower control flow must insert an S_AND_B64
 ; with exec.
 
 ; GCN-LABEL: {{^}}needs_and:
-; GCN: s_xor_b64 [[REG1:[^ ,]*]], {{[^ ,]*, -1$}}
-; GCN: s_and_b64 [[REG2:[^ ,]*]], exec, [[REG1]]
-; GCN: s_or_b64 [[REG3:[^ ,]*]], [[REG2]],
-; GCN: s_andn2_b64 exec, exec, [[REG3]]
 
+; GCN: s_or_b64 exec, exec, [[REG1:[^ ,]*]]
+; GCN: s_andn2_b64 exec, exec, [[REG2:[^ ,]*]]
+; GCN: s_or_b64 [[REG2:[^ ,]*]], [[REG1:[^ ,]*]], [[REG2:[^ ,]*]]
+; GCN: s_or_b64 exec, exec, [[REG2:[^ ,]*]]
 define void @needs_and(i32 %arg) {
 entry:
   br label %loop
 
 loop:
   %tmp23phi = phi i32 [ %tmp23, %endif ], [ 0, %entry ]
   %tmp23 = add nuw i32 %tmp23phi, 1
   %tmp27 = icmp ult i32 %arg, %tmp23

llvm/test/CodeGen/AMDGPU/multilevel-break.ll

 ; GCN-NEXT:    s_or_b64 exec, exec, s[8:9]
 ; GCN-NEXT:    s_and_b64 s[8:9], exec, s[6:7]
 ; GCN-NEXT:    s_or_b64 s[4:5], s[8:9], s[4:5]
 ; GCN-NEXT:    s_andn2_b64 exec, exec, s[4:5]
 ; GCN-NEXT:    s_cbranch_execz BB0_1
 ; GCN-NEXT:  BB0_4: ; %LOOP
 ; GCN-NEXT:    ; Parent Loop BB0_2 Depth=1
 ; GCN-NEXT:    ; => This Inner Loop Header: Depth=2
-; GCN-NEXT:    v_mov_b32_e32 v1, v0
-; GCN-NEXT:    v_add_i32_e32 v0, vcc, 1, v1
-; GCN-NEXT:    v_cmp_lt_i32_e32 vcc, v1, v4
+; GCN-NEXT:    v_cmp_lt_i32_e32 vcc, v0, v4
 ; GCN-NEXT:    s_or_b64 s[2:3], s[2:3], exec
 ; GCN-NEXT:    s_or_b64 s[6:7], s[6:7], exec
 ; GCN-NEXT:    s_and_saveexec_b64 s[8:9], vcc
 ; GCN-NEXT:    s_cbranch_execz BB0_3
 ; GCN-NEXT:  ; %bb.5: ; %ENDIF
 ; GCN-NEXT:    ; in Loop: Header=BB0_4 Depth=2
-; GCN-NEXT:    v_cmp_ne_u32_e32 vcc, v5, v0
+; GCN-NEXT:    v_add_i32_e32 v0, vcc, 1, v0
 ; GCN-NEXT:    s_andn2_b64 s[2:3], s[2:3], exec
+; GCN-NEXT:    v_cmp_ne_u32_e32 vcc, v5, v0
 ; GCN-NEXT:    s_andn2_b64 s[6:7], s[6:7], exec
 ; GCN-NEXT:    s_and_b64 s[10:11], vcc, exec
 ; GCN-NEXT:    s_or_b64 s[6:7], s[6:7], s[10:11]
 ; GCN-NEXT:    s_branch BB0_3
 ; GCN-NEXT:  BB0_6: ; %IF
 ; GCN-NEXT:    s_endpgm
 main_body:
   br label %LOOP.outer

 ; GCN-NEXT:    s_or_b64 s[0:1], s[6:7], s[0:1]
 ; GCN-NEXT:    s_andn2_b64 s[4:5], s[4:5], exec
 ; GCN-NEXT:    s_and_b64 s[6:7], s[8:9], exec
 ; GCN-NEXT:    s_or_b64 s[4:5], s[4:5], s[6:7]
 ; GCN-NEXT:    s_andn2_b64 exec, exec, s[0:1]
 ; GCN-NEXT:    s_cbranch_execz BB1_9
 ; GCN-NEXT:  BB1_2: ; %bb1
 ; GCN-NEXT:    ; =>This Inner Loop Header: Depth=1
-; GCN-NEXT:    buffer_load_dword v1, off, s[0:3], 0
+; GCN-NEXT:    buffer_load_dword v1, off, s[0:3], 0 glc
 ; GCN-NEXT:    s_waitcnt vmcnt(0)
 ; GCN-NEXT:    v_cmp_gt_i32_e32 vcc, 1, v1
 ; GCN-NEXT:    s_mov_b64 s[6:7], -1
 ; GCN-NEXT:    s_and_b64 vcc, exec, vcc
 ; GCN-NEXT:    ; implicit-def: $sgpr8_sgpr9
 ; GCN-NEXT:    s_mov_b64 s[10:11], -1
 ; GCN-NEXT:    s_cbranch_vccnz BB1_6
 ; GCN-NEXT:  ; %bb.3: ; %LeafBlock1
 ; GCN-NEXT:    ; in Loop: Header=BB1_2 Depth=1
 ; GCN-NEXT:    s_mov_b64 s[6:7], -1
 ; GCN-NEXT:    v_cmp_eq_u32_e32 vcc, 1, v1
 ; GCN-NEXT:    s_and_b64 vcc, exec, vcc
 ; GCN-NEXT:    s_mov_b64 s[8:9], -1
 ; GCN-NEXT:    s_cbranch_vccz BB1_5
 ; GCN-NEXT:  ; %bb.4: ; %case1
 ; GCN-NEXT:    ; in Loop: Header=BB1_2 Depth=1
-; GCN-NEXT:    buffer_load_dword v2, off, s[0:3], 0
+; GCN-NEXT:    buffer_load_dword v2, off, s[0:3], 0 glc
 ; GCN-NEXT:    s_waitcnt vmcnt(0)
 ; GCN-NEXT:    v_cmp_ge_i32_e32 vcc, v0, v2
 ; GCN-NEXT:    s_mov_b64 s[8:9], 0
 ; GCN-NEXT:    s_orn2_b64 s[6:7], vcc, exec
 ; GCN-NEXT:  BB1_5: ; %Flow3
 ; GCN-NEXT:    ; in Loop: Header=BB1_2 Depth=1
 ; GCN-NEXT:    s_mov_b64 s[10:11], 0
 ; GCN-NEXT:  BB1_6: ; %Flow
 ; GCN-NEXT:    ; in Loop: Header=BB1_2 Depth=1
 ; GCN-NEXT:    s_and_b64 vcc, exec, s[10:11]
 ; GCN-NEXT:    s_cbranch_vccz BB1_1
 ; GCN-NEXT:  ; %bb.7: ; %LeafBlock
 ; GCN-NEXT:    ; in Loop: Header=BB1_2 Depth=1
 ; GCN-NEXT:    v_cmp_eq_u32_e32 vcc, 0, v1
 ; GCN-NEXT:    s_and_b64 vcc, exec, vcc
 ; GCN-NEXT:    s_mov_b64 s[8:9], -1
 ; GCN-NEXT:    s_cbranch_vccz BB1_1
 ; GCN-NEXT:  ; %bb.8: ; %case0
 ; GCN-NEXT:    ; in Loop: Header=BB1_2 Depth=1
-; GCN-NEXT:    buffer_load_dword v1, off, s[0:3], 0
+; GCN-NEXT:    buffer_load_dword v1, off, s[0:3], 0 glc
 ; GCN-NEXT:    s_waitcnt vmcnt(0)
 ; GCN-NEXT:    s_mov_b64 s[8:9], 0
 ; GCN-NEXT:    v_cmp_ge_i32_e32 vcc, v0, v1
 ; GCN-NEXT:    s_andn2_b64 s[6:7], s[6:7], exec
 ; GCN-NEXT:    s_and_b64 s[10:11], vcc, exec
 ; GCN-NEXT:    s_or_b64 s[6:7], s[6:7], s[10:11]
 ; GCN-NEXT:    s_branch BB1_1
 ; GCN-NEXT:  BB1_9: ; %loop.exit.guard