[MachineSink] Don't reject sinking because of a dead def in isProfitableToSinkTo()

[jonpa]

Summary

I am not quite sure, but it seems that the intent in this profitability check may be to avoid sinking phys-reg uses as they would prolong live ranges.

It seems reasonable to me to sink an instruction regardless if it has a dead def of a physreg or not. Physreg defs are checked in other places and sinking is only done with dead defs of regs that are not live into the target MBB.

It may be that this has more than a negligble impact on code generation, so perhaps this was avoided purposefully. It seems to change quite a lot of files on SPEC/SystemZ, at least. On the other hand, it seems that dead defs are in fact allowed generally. Please fill me in if I am missing something here.

This came up when I was experimenting with a pseudo that has the same operands as the target instruction, except it pretends to clobber CC. At least in this case I don't see why the sinking of that instruction should be any different than without that fake dead CC def.

[arsenm]

llvm/test/CodeGen/AMDGPU/loop_exit_with_xor.ll

2

Switching a test to generated checks should be done in precommits

[jonpa]

llvm/test/CodeGen/AMDGPU/loop_exit_with_xor.ll

2

ok - I will do so if there is a positive response to this patch, and if it's ok with you (AMD maintainer).

[sebastian-ne]

Sounds like a good idea to me, but I’m not versed enough in MachineIR to put the accept stamp on it.

llvm/lib/CodeGen/MachineSink.cpp

809–813

Should probably add brackets around the multiline-if.

[alexfh]

As a part of our routine internal release testing we found out that the Ptrdist/yacr2 benchmark shows an around 10% regression after this commit on Haswell CPUs. It's not blocking us in any way, but you might be interested in confirming that this doesn't indicate an undesired side effect on the generated code.

[jonpa]

In D150447#4361413, @alexfh wrote:

As a part of our routine internal release testing we found out that the Ptrdist/yacr2 benchmark shows an around 10% regression after this commit on Haswell CPUs. It's not blocking us in any way, but you might be interested in confirming that this doesn't indicate an undesired side effect on the generated code.

It's hard to say exactly what the reason is for that regression, but my best guess would be increased spilling which I think could happen when you increase sinking. This patch only says that it shouldn't affect the sinking decision if there is a dead def of a phys reg which is not live into the target MBB. That shouldn't affect reg-pressure / spilling by itself. Maybe this increased spilling (if that is indeed the case) comes from other factors which would then call for an additional further refinement of this cost function. Or it could be something else that needs fixing.

I checked impact on SystemZ / SPEC, which looked fine (no big changes). Don't know about yacr2 on Haswell - somebody more familiar would need to look into that...

llvm/lib/CodeGen/MachineSink.cpp

for (const MachineOperand &MO : MI.operands()) {
     // Ignore non-register operands.
     if (!MO.isReg())
       continue;
     Register Reg = MO.getReg();
     if (Reg == 0)
       continue;
 
     if (Reg.isPhysical()) {
-      if (MO.isUse() &&
-          (MRI->isConstantPhysReg(Reg) || TII->isIgnorableUse(MO)))
-        continue;
-
-      // Don't handle non-constant and non-ignorable physical register.
+      // Don't handle non-constant and non-ignorable physical register uses.
+      if (MO.isUse() && !MRI->isConstantPhysReg(Reg) && !TII->isIgnorableUse(MO))
         return false;
+      continue;
     }
 
     // Users for the defs are all dominated by SuccToSinkTo.
     if (MO.isDef()) {
       // This def register's live range is shortened after sinking.
       bool LocalUse = false;
       if (!AllUsesDominatedByBlock(Reg, SuccToSinkTo, MBB, BreakPHIEdge,
                                    LocalUse))
         return false;

llvm/test/CodeGen/AMDGPU/collapse-endcf.ll

 ; GCN-NEXT:    s_mov_b64 s[6:7], 0
 ; GCN-NEXT:    s_branch .LBB5_3
 ; GCN-NEXT:  .LBB5_1: ; %Flow
 ; GCN-NEXT:    ; in Loop: Header=BB5_3 Depth=1
 ; GCN-NEXT:    s_or_b64 exec, exec, s[10:11]
 ; GCN-NEXT:  .LBB5_2: ; %bb10
 ; GCN-NEXT:    ; in Loop: Header=BB5_3 Depth=1
 ; GCN-NEXT:    s_or_b64 exec, exec, s[14:15]
+; GCN-NEXT:    s_and_b64 s[6:7], exec, s[4:5]
+; GCN-NEXT:    s_or_b64 s[12:13], s[6:7], s[12:13]
 ; GCN-NEXT:    s_mov_b64 s[6:7], 0
 ; GCN-NEXT:    s_andn2_b64 exec, exec, s[12:13]
 ; GCN-NEXT:    s_cbranch_execz .LBB5_7
 ; GCN-NEXT:  .LBB5_3: ; %bb1
 ; GCN-NEXT:    ; =>This Inner Loop Header: Depth=1
 ; GCN-NEXT:    s_and_b64 s[10:11], exec, vcc
 ; GCN-NEXT:    s_or_b64 s[6:7], s[10:11], s[6:7]
 ; GCN-NEXT:    s_andn2_b64 exec, exec, s[6:7]
 ; GCN-NEXT:    s_cbranch_execnz .LBB5_3
 ; GCN-NEXT:  ; %bb.4: ; %bb2
 ; GCN-NEXT:    ; in Loop: Header=BB5_3 Depth=1
 ; GCN-NEXT:    s_or_b64 exec, exec, s[6:7]
-; GCN-NEXT:    s_and_b64 s[6:7], exec, s[4:5]
 ; GCN-NEXT:    s_mov_b32 s9, s8
 ; GCN-NEXT:    s_mov_b32 s10, s8
 ; GCN-NEXT:    s_mov_b32 s11, s8
 ; GCN-NEXT:    v_mov_b32_e32 v0, s8
-; GCN-NEXT:    s_or_b64 s[12:13], s[6:7], s[12:13]
 ; GCN-NEXT:    v_mov_b32_e32 v1, s9
 ; GCN-NEXT:    v_mov_b32_e32 v2, s10
 ; GCN-NEXT:    v_mov_b32_e32 v3, s11
 ; GCN-NEXT:    s_and_saveexec_b64 s[14:15], s[4:5]
 ; GCN-NEXT:    s_cbranch_execz .LBB5_2
 ; GCN-NEXT:  ; %bb.5: ; %bb4
 ; GCN-NEXT:    ; in Loop: Header=BB5_3 Depth=1
 ; GCN-NEXT:    buffer_load_dword v0, v0, s[0:3], 0 offen

llvm/test/CodeGen/AMDGPU/loop_exit_with_xor.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
 ; 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.
 
 define void @needs_and(i32 %arg) {
 ; GCN-LABEL: needs_and:
 ; GCN:       ; %bb.0: ; %entry
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GCN-NEXT:    s_mov_b32 s8, 1
+; GCN-NEXT:    s_mov_b32 s10, 1
 ; GCN-NEXT:    s_mov_b64 s[6:7], 0
 ; GCN-NEXT:    s_branch .LBB0_2
 ; GCN-NEXT:  .LBB0_1: ; %endif
 ; GCN-NEXT:    ; in Loop: Header=BB0_2 Depth=1
-; GCN-NEXT:    s_or_b64 exec, exec, s[4:5]
-; GCN-NEXT:    s_add_i32 s8, s8, 1
+; GCN-NEXT:    s_or_b64 exec, exec, s[8:9]
+; GCN-NEXT:    s_and_b64 s[4:5], exec, vcc
+; GCN-NEXT:    s_or_b64 s[6:7], s[4:5], s[6:7]
+; GCN-NEXT:    s_add_i32 s10, s10, 1
 ; GCN-NEXT:    s_andn2_b64 exec, exec, s[6:7]
 ; GCN-NEXT:    s_cbranch_execz .LBB0_4
 ; GCN-NEXT:  .LBB0_2: ; %loop
 ; GCN-NEXT:    ; =>This Inner Loop Header: Depth=1
-; GCN-NEXT:    v_cmp_le_u32_e64 s[4:5], s8, v0
-; GCN-NEXT:    v_cmp_gt_u32_e32 vcc, s8, v0
-; GCN-NEXT:    s_or_b64 s[6:7], s[4:5], s[6:7]
-; GCN-NEXT:    s_and_saveexec_b64 s[4:5], vcc
+; GCN-NEXT:    v_cmp_gt_u32_e64 s[4:5], s10, v0
+; GCN-NEXT:    v_cmp_le_u32_e32 vcc, s10, v0
+; GCN-NEXT:    s_and_saveexec_b64 s[8:9], s[4:5]
 ; GCN-NEXT:    s_cbranch_execz .LBB0_1
 ; GCN-NEXT:  ; %bb.3: ; %then
 ; GCN-NEXT:    ; in Loop: Header=BB0_2 Depth=1
-; GCN-NEXT:    s_nop 0
+; GCN-NEXT:    s_nop 1
 ; GCN-NEXT:    buffer_store_dword v0, off, s[4:7], s4
 ; GCN-NEXT:    s_branch .LBB0_1
 ; GCN-NEXT:  .LBB0_4: ; %loopexit
 ; GCN-NEXT:    s_or_b64 exec, exec, s[6:7]
 ; GCN-NEXT:    s_waitcnt vmcnt(0)
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 entry:
   br label %loop

 ; GCN-NEXT:    v_cmp_eq_u32_e32 vcc, 1, v1
 ; GCN-NEXT:    s_xor_b64 s[4:5], vcc, -1
 ; GCN-NEXT:    s_mov_b32 s10, 1
 ; GCN-NEXT:    s_mov_b64 s[6:7], 0
 ; GCN-NEXT:    s_branch .LBB2_2
 ; GCN-NEXT:  .LBB2_1: ; %endif
 ; GCN-NEXT:    ; in Loop: Header=BB2_2 Depth=1
 ; GCN-NEXT:    s_or_b64 exec, exec, s[8:9]
+; GCN-NEXT:    s_and_b64 s[8:9], exec, s[4:5]
+; GCN-NEXT:    s_or_b64 s[6:7], s[8:9], s[6:7]
 ; GCN-NEXT:    s_add_i32 s10, s10, 1
 ; GCN-NEXT:    s_andn2_b64 exec, exec, s[6:7]
 ; GCN-NEXT:    s_cbranch_execz .LBB2_4
 ; GCN-NEXT:  .LBB2_2: ; %loop
 ; GCN-NEXT:    ; =>This Inner Loop Header: Depth=1
-; GCN-NEXT:    s_and_b64 s[8:9], exec, s[4:5]
-; GCN-NEXT:    s_or_b64 s[6:7], s[8:9], s[6:7]
 ; GCN-NEXT:    v_cmp_gt_u32_e32 vcc, s10, v0
 ; GCN-NEXT:    s_and_saveexec_b64 s[8:9], vcc
 ; GCN-NEXT:    s_cbranch_execz .LBB2_1
 ; GCN-NEXT:  ; %bb.3: ; %then
 ; GCN-NEXT:    ; in Loop: Header=BB2_2 Depth=1
 ; GCN-NEXT:    buffer_store_dword v0, off, s[4:7], s4
 ; GCN-NEXT:    s_branch .LBB2_1
 ; GCN-NEXT:  .LBB2_4: ; %loopexit

llvm/test/CodeGen/AMDGPU/should-not-hoist-set-inactive.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
 ; RUN: llc -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck %s -check-prefix=GCN
 
 define amdgpu_cs void @should_not_hoist_set_inactive(<4 x i32> inreg %i14, i32 inreg %v, i32 %lane, i32 %f, i32 %f2) #0 {
 ; GCN-LABEL: should_not_hoist_set_inactive:
 ; GCN:       ; %bb.0: ; %.entry
 ; GCN-NEXT:    v_cmp_gt_i32_e32 vcc_lo, 3, v1
 ; GCN-NEXT:    v_cmp_eq_u32_e64 s5, 0, v0
 ; GCN-NEXT:    v_cmp_ne_u32_e64 s6, 0, v2
 ; GCN-NEXT:    s_mov_b32 s7, 0
 ; GCN-NEXT:    s_branch .LBB0_2
 ; GCN-NEXT:  .LBB0_1: ; %bb4
 ; GCN-NEXT:    ; in Loop: Header=BB0_2 Depth=1
 ; GCN-NEXT:    s_waitcnt_depctr 0xffe3
 ; GCN-NEXT:    s_or_b32 exec_lo, exec_lo, s8
+; GCN-NEXT:    s_and_b32 s8, exec_lo, s6
+; GCN-NEXT:    s_or_b32 s7, s8, s7
 ; GCN-NEXT:    s_andn2_b32 exec_lo, exec_lo, s7
 ; GCN-NEXT:    s_cbranch_execz .LBB0_5
 ; GCN-NEXT:  .LBB0_2: ; %bb
 ; GCN-NEXT:    ; =>This Inner Loop Header: Depth=1
-; GCN-NEXT:    s_and_b32 s8, exec_lo, s6
-; GCN-NEXT:    s_or_b32 s7, s8, s7
 ; GCN-NEXT:    s_and_saveexec_b32 s8, vcc_lo
 ; GCN-NEXT:    s_cbranch_execz .LBB0_1
 ; GCN-NEXT:  ; %bb.3: ; %bb1
 ; GCN-NEXT:    ; in Loop: Header=BB0_2 Depth=1
 ; GCN-NEXT:    v_mov_b32_e32 v3, s4
 ; GCN-NEXT:    s_not_b32 exec_lo, exec_lo
 ; GCN-NEXT:    v_mov_b32_e32 v3, 0
 ; GCN-NEXT:    s_not_b32 exec_lo, exec_lo

llvm/test/CodeGen/SystemZ/machinesink-dead-cc.mir

+# RUN: llc -mtriple=s390x-linux-gnu -mcpu=z15 -O3 -run-pass=machine-sink %s -o - \
+# RUN:   -verify-machineinstrs | FileCheck %s
+#
+# Test that the AGHIK can be sunk into %bb.4. It has a def of CC, but it is dead.
+
+--- |
+  define void @fun() {  ret void  }
+...
+
+# CHECK-LABEL: bb.4:
+# CHECK:         %1:gr64bit = nsw AGHIK %0, -4, implicit-def dead $cc
+# CHECK-NEXT:    CGHI %1, 0, implicit-def $cc
+# CHECK-NEXT:    BRC 14, 6, %bb.1, implicit $cc
+# CHECK-NEXT:    J %bb.5
+
+
+---
+name:            fun
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr64bit }
+  - { id: 1, class: gr64bit }
+  - { id: 2, class: grx32bit }
+  - { id: 3, class: gr64bit }
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+
+    %2:grx32bit = LHIMux 0
+    %3:gr64bit = IMPLICIT_DEF
+
+  bb.1:
+
+    %0:gr64bit = PHI %3, %bb.0, %1, %bb.4
+  
+  bb.2:
+  
+    %1:gr64bit = nsw AGHIK %0, -4, implicit-def dead $cc
+    CHIMux %2, 0, implicit-def $cc
+    BRC 14, 6, %bb.4, implicit $cc
+    J %bb.3
+  
+  bb.3:
+  
+  bb.4:
+  
+    CGHI %1, 0, implicit-def $cc
+    BRC 14, 6, %bb.1, implicit $cc
+    J %bb.5
+  
+  bb.5:
+    Return
+
+...

llvm/test/CodeGen/X86/2007-01-13-StackPtrIndex.ll

 ; CHECK-NEXT:    je .LBB0_19
 ; CHECK-NEXT:  .LBB0_4: # %a33b
 ; CHECK-NEXT:    # in Loop: Header=BB0_3 Depth=1
 ; CHECK-NEXT:    movl %esi, %r9d
 ; CHECK-NEXT:    orl %eax, %r9d
 ; CHECK-NEXT:    jns .LBB0_20
 ; CHECK-NEXT:  .LBB0_5: # %a50b
 ; CHECK-NEXT:    # in Loop: Header=BB0_3 Depth=1
-; CHECK-NEXT:    shrl $31, %r9d
 ; CHECK-NEXT:    movl %eax, %r10d
 ; CHECK-NEXT:    orl %esi, %r10d
 ; CHECK-NEXT:    jns .LBB0_26
 ; CHECK-NEXT:  .LBB0_6: # %a57b
 ; CHECK-NEXT:    # in Loop: Header=BB0_3 Depth=1
-; CHECK-NEXT:    shrl $31, %r10d
+; CHECK-NEXT:    shrl $31, %r9d
 ; CHECK-NEXT:    testb %r9b, %r9b
 ; CHECK-NEXT:    je .LBB0_30
 ; CHECK-NEXT:  .LBB0_7: # %a66b
 ; CHECK-NEXT:    # in Loop: Header=BB0_3 Depth=1
+; CHECK-NEXT:    shrl $31, %r10d
 ; CHECK-NEXT:    testb %r10b, %r10b
 ; CHECK-NEXT:    jne .LBB0_8
 ; CHECK-NEXT:    .p2align 4, 0x90
 ; CHECK-NEXT:  .LBB0_34: # %a74b
 ; CHECK-NEXT:    # Parent Loop BB0_3 Depth=1
 ; CHECK-NEXT:    # => This Inner Loop Header: Depth=2
 ; CHECK-NEXT:    testb %dil, %dil
 ; CHECK-NEXT:    jne .LBB0_34

llvm/test/CodeGen/X86/dag-update-nodetomatch.ll

 ; CHECK-NEXT:  # %bb.6: # %vector.memcheck
 ; CHECK-NEXT:    # in Loop: Header=BB1_2 Depth=1
 ; CHECK-NEXT:    addq {{[-0-9]+}}(%r{{[sb]}}p), %r10 # 8-byte Folded Reload
 ; CHECK-NEXT:    leaq (%r10,%r11,8), %r8
 ; CHECK-NEXT:    cmpq %r12, %r8
 ; CHECK-NEXT:    ja .LBB1_14
 ; CHECK-NEXT:  .LBB1_7: # %vector.body.preheader
 ; CHECK-NEXT:    # in Loop: Header=BB1_2 Depth=1
-; CHECK-NEXT:    leaq -4(%rcx), %r8
-; CHECK-NEXT:    movq %r8, %r11
-; CHECK-NEXT:    shrq $2, %r11
-; CHECK-NEXT:    btl $2, %r8d
+; CHECK-NEXT:    leaq -4(%rcx), %r11
+; CHECK-NEXT:    btl $2, %r11d
 ; CHECK-NEXT:    jb .LBB1_8
 ; CHECK-NEXT:  # %bb.9: # %vector.body.prol.preheader
 ; CHECK-NEXT:    # in Loop: Header=BB1_2 Depth=1
 ; CHECK-NEXT:    movq {{.*#+}} xmm0 = mem[0],zero
 ; CHECK-NEXT:    pshufd {{.*#+}} xmm0 = xmm0[0,1,0,1]
 ; CHECK-NEXT:    movdqu %xmm0, (%r13,%rbp,8)
 ; CHECK-NEXT:    movdqu %xmm0, 16(%r13,%rbp,8)
 ; CHECK-NEXT:    movl $4, %r10d
-; CHECK-NEXT:    testq %r11, %r11
+; CHECK-NEXT:    shrq $2, %r11
 ; CHECK-NEXT:    jne .LBB1_11
 ; CHECK-NEXT:    jmp .LBB1_13
 ; CHECK-NEXT:  .LBB1_8: # in Loop: Header=BB1_2 Depth=1
 ; CHECK-NEXT:    xorl %r10d, %r10d
-; CHECK-NEXT:    testq %r11, %r11
+; CHECK-NEXT:    shrq $2, %r11
 ; CHECK-NEXT:    je .LBB1_13
 ; CHECK-NEXT:  .LBB1_11: # %vector.body.preheader.new
 ; CHECK-NEXT:    # in Loop: Header=BB1_2 Depth=1
 ; CHECK-NEXT:    movq {{.*#+}} xmm0 = mem[0],zero
 ; CHECK-NEXT:    pshufd {{.*#+}} xmm0 = xmm0[0,1,0,1]
 ; CHECK-NEXT:    movq %r10, %r11
 ; CHECK-NEXT:    subq %rcx, %r11
 ; CHECK-NEXT:    addq %rbp, %r10

llvm/test/CodeGen/X86/statepoint-cmp-sunk-past-statepoint.ll

 ; CHECK:      bb.3
 ; CHECK:        %18:gr8 = COPY %17.sub_8bit
 ; CHECK-LV:     TEST8rr killed %18, %18, implicit-def $eflags
 ; CHECK-LIS:    TEST8rr %18, %18, implicit-def $eflags
 ; CHECK:        JCC_1 %bb.5, 5, implicit killed $eflags
 ; CHECK:        JMP_1 %bb.4
 ; CHECK:      bb.4
 ; CHECK:      bb.5
-; CHECK:        %4:gr64 = LEA64r %10, 1, $noreg, 8, $noreg
-; CHECK-LV:     %3:gr64 = COPY killed %10
-; CHECK-LIS:    %3:gr64 = COPY %10
-; CHECK-LV:     TEST64rr killed %1, %1, implicit-def $eflags
+; CHECK:        %3:gr64 = COPY %10
+; CHECK-LV:     %4:gr64 = COPY killed %10
+; CHECK-LV:     %4:gr64 = nuw ADD64ri8 %4, 8, implicit-def dead $eflags
+; CHECK-LIS:    %4:gr64 = LEA64r %10, 1, $noreg, 8, $noreg
+; CHECK:        TEST64rr killed %1, %1, implicit-def $eflags
 ; CHECK:        JCC_1 %bb.1, 5, implicit killed $eflags
 ; CHECK:        JMP_1 %bb.6
 define void @test2(i8 addrspace(1)* %this, i32 %0, i32 addrspace(1)* %p0, i8 addrspace(1)* %p1) gc "statepoint-example" personality i32* ()* @fake_personality_function {
 preheader:
   br label %loop.head
 
 loop.head:
   %phi1 = phi i32 addrspace(1)* [ %p0, %preheader ], [ %addr.i.i46797.remat64523, %tail ]