MachineSink debug invariance

[markus]

Summary

https://github.com/llvm/llvm-project/issues/55914

When counting instructions to compare with threshold debug instructions need to be ignored in order to be invariant wrt to debuginfo

[markus]

Should add a .mir test

llvm/lib/CodeGen/MachineSink.cpp

271

Don't we already have some utility function for this?

[Orlando]

Should add a .mir test

+1

This change looks ok to me but FTR I haven't touched the MachineSink pass before.

llvm/lib/CodeGen/MachineSink.cpp

271

It looks like we have .instructionsWithoutDebug() and .sizeWithoutDebug() for BasicBlock but I'm not sure if there's an equivalent for MachineBasicBlock. Maybe it'd be worth adding to MachineBasicBlock?

274

Should this be .isMetaInstr() (rather than .isDebugInstr()) to cover other non-debug that do not correspond to executable instructions?

[markus]

Add MachineBasicBlock::sizeWithoutDebug() to make more similar to IR.

[markus]

llvm/lib/CodeGen/MachineSink.cpp

274

Not sure but the way we do it now should be equivalent to how it works in BasicBlock::sizeWithoutDebug() which I guess is the way it should be.

[Orlando]

The code changes look good to me, so just waiting for the test before approving now.

[jmorse]

Also LGTM with a regression test, thanks for finding this.

[fhahn]

llvm/lib/CodeGen/MachineBasicBlock.cpp

1622 ↗

(On Diff #434806)

One issue with this helper is that it needs to iterate over all instructions in the block to count them and the limit is used to decide whether the block is too big to process.

Not sure there's a good alternative, but maybe have something like sizeWIthoutDebugLargerThan(x) so we can at least bail out once the limit is reached.

[jmorse]

llvm/lib/CodeGen/MachineBasicBlock.cpp

1622 ↗

(On Diff #434806)

IIUC, basic block size is ilist size, which is already linear time anyway, so this wouldn't be a regression. That being said, putting a bound on this would be a neat extension.

[markus]

Added lit-test.

[markus]

llvm/lib/CodeGen/MachineBasicBlock.cpp

1622 ↗

(On Diff #434806)

Agree that it would be better to limit the number of instructions scanned but I worry that introducing such a function in MachineBasicBlock would make it slightly unsymmetric wrt its IR equivalent. Maybe it is worth it though, I can't really tell.

[Orlando]

The test LGTM - I'll leave it to @jmorse or @fhahn to accept when a consensus is reached on whether a bounded version of sizeWithoutDebug should be added or not.

[markus]

Fixed typo in lit-test.

[markus]

MachineBasicBlock::sizeWithoutDebugLargerThan(unsigned Limit)

[markus]

ping

[jmorse]

LGTM, adding a size-with-limit method looks good too.

llvm/lib/CodeGen/MachineSink.cpp

INITIALIZE_PASS_BEGIN(MachineSinking, DEBUG_TYPE,
                       "Machine code sinking", false, false)
 INITIALIZE_PASS_DEPENDENCY(MachineBranchProbabilityInfo)
 INITIALIZE_PASS_DEPENDENCY(MachineDominatorTree)
 INITIALIZE_PASS_DEPENDENCY(MachineCycleInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
 INITIALIZE_PASS_END(MachineSinking, DEBUG_TYPE,
                     "Machine code sinking", false, false)
 
+static unsigned sizeNoDbg(const MachineBasicBlock &MBB) {
+  unsigned Size = 0;
+  for (auto &MI : MBB) {
+    if (MI.isDebugInstr())
+      continue;
+    Size++;
+  }
+  return Size;
+}
+
 bool MachineSinking::PerformTrivialForwardCoalescing(MachineInstr &MI,
                                                      MachineBasicBlock *MBB) {
   if (!MI.isCopy())
     return false;
 
   Register SrcReg = MI.getOperand(1).getReg();
   Register DstReg = MI.getOperand(0).getReg();
   if (!Register::isVirtualRegister(SrcReg) ||
for (MachineBasicBlock *BB : depth_first(From)) {
     HandledBlocks.insert(BB);
     // To post dominates BB, it must be a path from block From.
     if (PDT->dominates(To, BB)) {
       if (!HandledDomBlocks.count(BB))
         HandledDomBlocks.insert(BB);
 
       // If this BB is too big or the block number in straight line between From
       // and To is too big, stop searching to save compiling time.
-      if (BB->size() > SinkLoadInstsPerBlockThreshold ||
+      if (sizeNoDbg(*BB) > SinkLoadInstsPerBlockThreshold ||
           HandledDomBlocks.size() > SinkLoadBlocksThreshold) {
         for (auto *DomBB : HandledDomBlocks) {
           if (DomBB != BB && DT->dominates(DomBB, BB))
             HasStoreCache[std::make_pair(DomBB, To)] = true;
           else if(DomBB != BB && DT->dominates(BB, DomBB))
             HasStoreCache[std::make_pair(From, DomBB)] = true;
         }
         HasStoreCache[BlockPair] = true;
if (!SinkBlock) {
     LLVM_DEBUG(dbgs() << "CycleSink: Not sinking, can't find sink block.\n");
     return false;
   }
   if (SinkBlock == Preheader) {
     LLVM_DEBUG(
         dbgs() << "CycleSink: Not sinking, sink block is the preheader\n");
     return false;
   }
-  if (SinkBlock->size() > SinkLoadInstsPerBlockThreshold) {
+  if (sizeNoDbg(*SinkBlock) > SinkLoadInstsPerBlockThreshold) {
     LLVM_DEBUG(
         dbgs() << "CycleSink: Not Sinking, block too large to analyse.\n");
     return false;
   }
 
   LLVM_DEBUG(dbgs() << "CycleSink: Sinking instruction!\n");
   SinkBlock->splice(SinkBlock->SkipPHIsAndLabels(SinkBlock->begin()), Preheader,
                     I);