[MachineSink] add one more profitable pattern for sinking

[shchenz]

Summary

This patch adds one profitable pattern to sink MI when MI->parent() dominates SuccToSinkTo and SuccToSinkTo post dominates MI->parent().

If MI is in loop and MI is:

def0, def1, defn = opcode, use0, use1, use2, use3

sinking MI to SuccToSinkTo will shorten the live range for def but will enlarge the live range for use.
But in loop, the use may be defined outside of loop, so if SuccToSinkTo is still in same loop, sinking MI should have no impact for the live range for the uses which are defined out of loop.

If we know that all the uses are defined outside of loop, it is still profitable to sink.

[qcolombet]

llvm/lib/CodeGen/MachineSink.cpp

603

Could you invert the logic to early exit and reduce the indentation here?

611

Reg == 0, should be fine. We should just skip them.

621

Instead of counting the number of live-ranges, we should probably count their weights with respect to the related register pressure set.

E.g., <smallDef>, <smallDef> = op <BigUse>

Maybe each smallDef accounts for 1 register whereas BigUse accounts for 4 registers.

[shchenz]

use registerclassweight

[shchenz]

llvm/lib/CodeGen/MachineSink.cpp

621

Good idea!

[shchenz]

gentle ping

[qcolombet]

Hi,

The update with the RegClassWeight is only partially done IMHO.
Each weight should be added/removed from the related register pressure sets. You can take a look at MachineLICM.cpp to see how to use these.

I.e., even if the total cost decreases, this doesn't guarantee that we won't go over the limit of a pressure set and thus, we would spill.
E.g.,

loop:
   smallDef =
   ... = smallDef
   bigDef = smallDef
   // lot of small defs.
  condbr loop

Sinking bigDef would be seen profitable since the weight of bigDef is greater than smallDef. However, extending smallDef will make it overlaps with a lot of small defs and may create spilling (remember that smallDef and bigDef could be on different register banks.)

For now I would suggest to either:

1. Implement the support of pressure sets, or
2. Only do the transformation when the def of the uses are outside the loop. Like you said, these won't have any impact on the register pressure.

Random though: currently MachineLICM only does hoisting. I think it would make sense to have it also do sinking.
Put differently, conceptually, given we are looking at loop invariant here, it would make sense to have this code in MachineLICM instead of MachineSink.

What do you think?

Cheers,
-Quentin

[shchenz]

@qcolombet Thanks for your good comments.

given we are looking at loop invariant here, it would make sense to have this code in MachineLICM instead of MachineSink

MachineLICM tries to move loop invariant instructions to outside of a loop(preheader of exit block). Here we want to move def closer to its use inside the loop.
If we want to support 'real' sink in MachineLICM(move loop invariant instructions to the exit block), it should always benefit for the loop's register pressure?

If we want to move def closer to its use inside the loop to benefit register pressure in MachineLICM pass, except many duplicated codes with machinesink pass, (condition checks in SinkInstruction), register estimate model in machineLICM also does not support this. I checked UpdateBackTraceRegPressure, seems now the model is designed for hoisting only. It just stores the register pressure on path from loop preheader to current BB. see RegPressure and calcRegisterCost(Sinking should have a different cost calculation). For sinking, we need to estimate the register pressure in the destination block which is not on path from loop preheader to current BB.

The real world case I met is not a loop invariant instruction, it has an operand which is a user of PHI in loop header.

So I think maybe we should implement this in machinesink pass?

But yes, the register pressure estimate is not accurate for now and your example shows this very well. If we want to estimate it accurately, maybe we can use struct RegisterPressure in RegisterPressure.h to check register pressure of the destinate BB after the sinking? If the register pressure for register pressure set exceeds the limit after the sinking, we don't sink. What do you think? @qcolombet

Thanks again for your comments.

[qcolombet]

The real world case I met is not a loop invariant instruction, it has an operand which is a user of PHI in loop header.

I see, yes, then having this in MachineSink makes sense. At least as a start :).

struct RegisterPressure is a good start, though I don't remember how it ties with the register pressure sets (TargetRegisterInfo::getRegClassPressureSets), which is ultimately what we want to use.

Given adding that is going to take some time, you could push your patch for the cases we know are always profitable: when the uses are alive across the whole loop (line 636 in the current implementation.)

Cheers,
-Quentin

[shchenz]

address @qcolombet comments:
1: firstly add the always profitable pattern

[shchenz]

Given adding that is going to take some time, you could push your patch for the cases we know are always profitable: when the uses are alive across the whole loop (line 636 in the current implementation.)

Thanks for this suggestion, updated this patch accordingly.
As expected, the improvement for the benchmark is gone now. : (
Will do the left part in following patch.

[qcolombet]

Hi,

As expected, the improvement for the benchmark is gone now. : (

Too bad. Hopefully it would take long to come up with the follow up path.

The source change looks good to me (there's one lint report that needs fixing though: one of the comment is larger than 80-col), but we miss a test case.

Could you add a test case that capture specifically what this patch is doing?
In other words, a test case that runs the machine sink pass alone and that fails without that patch and passes with that patch.

You can produce the skeleton of the test by running -stop-before machine-sink -simplify-mir, then you can use that output as input of your test with -run-pass=machine-sink.

Cheers,
-Quentin

[shchenz]

1: add one more mir test case
2: fix code format

[shchenz]

Thanks very much, updated, could you please help to have another look? @qcolombet

[qcolombet]

Hi @shchenz ,

1: add one more mir test case

Did you forget to do git add, I don't see it.

Cheers,
-Quentin

llvm/lib/CodeGen/MachineSink.cpp

622

This variable is a left over of the previous version of the patch.
Please delete it.

[shchenz]

delete unused variables

[shchenz]

1: add one more mir test case

Did you forget to do git add, I don't see it.

Weird, I added case test/CodeGen/PowerPC/sink-down-more-instructions-1.mir. Could you do another check?
Maybe we should delete the IR test test/CodeGen/PowerPC/sink-down-more-instructions.ll? it is same input with mir test.

[qcolombet]

Weird, I added case test/CodeGen/PowerPC/sink-down-more-instructions-1.mir. Could you do another check?

Ah sorry about that. I was expecting a new file, whereas the diff only shows a change in sinking for that file.

Up to you for the .ll file.

[shchenz]

Ah sorry about that. I was expecting a new file, whereas the diff only shows a change in sinking for that file.

To make the review easy, I pre-commit an NFC patch to add the test case. :)
Thanks very much for your patient review @qcolombet

llvm/lib/CodeGen/MachineSink.cpp

bool MachineSinking::isProfitableToSinkTo(unsigned Reg, MachineInstr &MI,
   // If SuccToSinkTo post dominates then also it may be profitable if MI
   // can further profitably sinked into another block in next round.
   bool BreakPHIEdge = false;
   // FIXME - If finding successor is compile time expensive then cache results.
   if (MachineBasicBlock *MBB2 =
           FindSuccToSinkTo(MI, SuccToSinkTo, BreakPHIEdge, AllSuccessors))
     return isProfitableToSinkTo(Reg, MI, SuccToSinkTo, MBB2, AllSuccessors);
 
-  // If SuccToSinkTo is final destination and it is a post dominator of current
-  // block then it is not profitable to sink MI into SuccToSinkTo block.
+  MachineLoop *ML = LI->getLoopFor(MBB);
+
+  // If the instruction is not inside a loop, it is not profitable to sink MI to
+  // a post dominate block SuccToSinkTo.
+  if (!ML)
+    return false;
+
+  // If this instruction is inside a loop and sinking this instruction can make
+  // more registers live range shorten, it is still prifitable.
+  for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
+    const MachineOperand &MO = MI.getOperand(i);
+    // Ignore non-register operands.
+    if (!MO.isReg())
+      continue;
+    Register Reg = MO.getReg();
+    if (Reg == 0)
+      continue;
+
+    // Don't handle physical register.
+    if (Register::isPhysicalRegister(Reg))
       return false;
+
+    const TargetRegisterClass *RC = MRI->getRegClass(Reg);
+    RegClassWeight W = TRI->getRegClassWeight(RC);
+    // 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;
+    } else {
+      MachineInstr *DefMI = MRI->getVRegDef(Reg);
+      // DefMI is defined outside of loop. There should be no live range
+      // impact for this operand. Defination outside of loop means:
+      // 1: defination is outside of loop.
+      // 2: defination is in this loop, but it is a PHI in the loop header.
+      if (LI->getLoopFor(DefMI->getParent()) != ML ||
+          (DefMI->isPHI() && LI->isLoopHeader(DefMI->getParent())))
+        continue;
+      // DefMI is inside the loop. Mark it as not profitable as sinking MI will
+      // enlarge DefMI live range.
+      // FIXME: check the register pressure in block SuccToSinkTo, if it is
+      // smaller than the limit after sinking, it is still profitable to sink.
+      return false;
+    }
+  }
+
+  // If MI is in loop and all its operands are alive across the whole loop, it is
+  // profitable to sink MI.
+  return true;
 }
 
 /// Get the sorted sequence of successors for this MachineBasicBlock, possibly
 /// computing it if it was not already cached.
 SmallVector<MachineBasicBlock *, 4> &
 MachineSinking::GetAllSortedSuccessors(MachineInstr &MI, MachineBasicBlock *MBB,
                                        AllSuccsCache &AllSuccessors) const {
   // Do we have the sorted successors in cache ?

llvm/test/CodeGen/PowerPC/sink-down-more-instructions.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN:  llc -mcpu=pwr9 -mtriple=powerpc64le-unknown-linux-gnu \
 ; RUN:      -ppc-asm-full-reg-names -verify-machineinstrs < %s | FileCheck %s
 
 define signext i32 @foo(i32 signext %0, i32 signext %1, i32* %2, i32* %3, i32 signext %4) {
 ; CHECK-LABEL: foo:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    cmpwi r7, 1
 ; CHECK-NEXT:    blt cr0, .LBB0_8
 ; CHECK-NEXT:  # %bb.1:
 ; CHECK-NEXT:    addi r4, r5, -4
-; CHECK-NEXT:    addi r8, r6, -4
-; CHECK-NEXT:    clrldi r7, r7, 32
+; CHECK-NEXT:    clrldi r8, r7, 32
+; CHECK-NEXT:    addi r7, r6, -4
 ; CHECK-NEXT:    li r5, 0
-; CHECK-NEXT:    mtctr r7
-; CHECK-NEXT:    lis r7, -30584
+; CHECK-NEXT:    mtctr r8
+; CHECK-NEXT:    lis r8, -30584
 ; CHECK-NEXT:    li r6, 0
 ; CHECK-NEXT:    cmplwi r3, 3
 ; CHECK-NEXT:    cmplwi cr1, r3, 1
-; CHECK-NEXT:    ori r7, r7, 34953
+; CHECK-NEXT:    ori r8, r8, 34953
 ; CHECK-NEXT:    b .LBB0_4
 ; CHECK-NEXT:    .p2align 4
 ; CHECK-NEXT:  .LBB0_2:
-; CHECK-NEXT:    mulhwu r9, r6, r7
-; CHECK-NEXT:    srwi r9, r9, 4
-; CHECK-NEXT:    mulli r9, r9, 30
-; CHECK-NEXT:    sub r9, r6, r9
+; CHECK-NEXT:    mulhwu r3, r6, r8
+; CHECK-NEXT:    srwi r3, r3, 4
+; CHECK-NEXT:    mulli r3, r3, 30
+; CHECK-NEXT:    sub r3, r6, r3
 ; CHECK-NEXT:  .LBB0_3:
 ; CHECK-NEXT:    addi r6, r6, 1
-; CHECK-NEXT:    add r9, r9, r5
-; CHECK-NEXT:    stw r9, 4(r8)
-; CHECK-NEXT:    mr r8, r3
+; CHECK-NEXT:    add r3, r3, r5
+; CHECK-NEXT:    stw r3, 4(r7)
+; CHECK-NEXT:    addi r7, r7, 4
 ; CHECK-NEXT:    bdz .LBB0_8
 ; CHECK-NEXT:  .LBB0_4:
-; CHECK-NEXT:    lwzu r9, 4(r4)
-; CHECK-NEXT:    addi r3, r8, 4
-; CHECK-NEXT:    add r5, r9, r5
+; CHECK-NEXT:    lwzu r3, 4(r4)
+; CHECK-NEXT:    add r5, r3, r5
 ; CHECK-NEXT:    beq cr0, .LBB0_7
 ; CHECK-NEXT:  # %bb.5:
 ; CHECK-NEXT:    bne cr1, .LBB0_2
 ; CHECK-NEXT:  # %bb.6:
-; CHECK-NEXT:    slwi r9, r6, 1
+; CHECK-NEXT:    slwi r3, r6, 1
 ; CHECK-NEXT:    b .LBB0_3
 ; CHECK-NEXT:    .p2align 4
 ; CHECK-NEXT:  .LBB0_7:
-; CHECK-NEXT:    addi r9, r6, 100
+; CHECK-NEXT:    addi r3, r6, 100
 ; CHECK-NEXT:    b .LBB0_3
 ; CHECK-NEXT:  .LBB0_8:
 ; CHECK-NEXT:    li r3, 0
 ; CHECK-NEXT:    blr
   %6 = icmp sgt i32 %4, 0
   br i1 %6, label %7, label %9
 
 7:                                                ; preds = %5