[AArch64InstrInfo] Ignore debug insts in canInstrSubstituteCmpInstr [5/10]

[vsk]

Summary

Fix an issue where the presence of debug info could disable a peephole
optimization in optimizeCompareInstr due to canInstrSubstituteCmpInstr
returning the wrong result.

Depends on D78137.

[paquette]

LGTM

[fhahn]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

1447

It seems like a few similar changes popped up recently. It might be worth making it a bit easier to iterate over a range of MIs while skipping debug instructions automatically. It's quite easy to construct iterator ranges that skip debug instructions (see snippet below). It might be worth adding a helpers like that. They could also naturally be used for the existing skipDebugInstructionsBackward/skipDebugInstructionsForward

+  auto MIsWithoutDbg =
+      make_filter_range(make_range(std::next(CmpInstr->getIterator()),
+                                   CmpInstr->getParent()->instr_end()),
+                        [](MachineInstr &I) { return !I.isDebugInstr(); });
+  for (const MachineInstr &Instr : MIsWithoutDbg) {

[paquette]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

1447

+1, it seems like this is an extremely easy mistake to make, and it's probably peppered around basically everywhere.

[vsk]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

1447

Thanks for bringing this up. It's not always feasible to use the range-loop idiom, but I agree that it's a good fit here. In cases where the loop iterator is either initialized before the for-init, or modified inside the for-body -- maybe not.

I experimented with some different approaches. Adding 'nodbg' filter_iterator variants for the various MachineBasicBlock iterators did not seem good to me, as it's quite intrusive: a lot of code has to change to adopt the new iterators. Adding helpers that replace std::{next, prev} (but skip debug instructions) could work, but adopting these turned out to be messier than I would've liked. In the end I chose to adapt what we have at the IR-level, but to allow for existing iterators to be passed in.

Zooming way out, I think the long-term plan should be to remove debug instructions from the instruction stream entirely. The recent callSiteInfo work proves this can work. A prerequisite step might be to have DBG_VALUE point to a MachineInstr, like @jmorse proposed in http://lists.llvm.org/pipermail/llvm-dev/2020-February/139440.html.

[vsk]

Introduce a range helper to visit instructions (skipping debug insts)

[vsk]

retitle, split out helpers into https://reviews.llvm.org/D78260

[fhahn]

LGTM, thanks

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

1447

I experimented with some different approaches. Adding 'nodbg' filter_iterator variants for the various MachineBasicBlock iterators did not seem good to me, as it's quite intrusive: a lot of code has to change to adopt the new iterators. Adding helpers that replace std::{next, prev} (but skip debug instructions) could work, but adopting these turned out to be messier than I would've liked. In the end I chose to adapt what we have at the IR-level, but to allow for existing iterators to be passed in.

That seems reasonable to me. Having custom start/end points for iterators seems much more common in the backend than in the middle end,

llvm/lib/CodeGen/PeepholeOptimizer.cpp

bool PeepholeOptimizer::optimizeCmpInstr(MachineInstr &MI) {
   // physical register, we can try to optimize it.
   Register SrcReg, SrcReg2;
   int CmpMask, CmpValue;
   if (!TII->analyzeCompare(MI, SrcReg, SrcReg2, CmpMask, CmpValue) ||
       SrcReg.isPhysical() || SrcReg2.isPhysical())
     return false;
 
   // Attempt to optimize the comparison instruction.
+  LLVM_DEBUG(dbgs() << "Attempting to optimize compare: " << MI);
   if (TII->optimizeCompareInstr(MI, SrcReg, SrcReg2, CmpMask, CmpValue, MRI)) {
     ++NumCmps;
     return true;
   }
 
   return false;
 }
 
 /// Optimize a select instruction.
 bool PeepholeOptimizer::optimizeSelect(MachineInstr &MI,
                             SmallPtrSetImpl<MachineInstr *> &LocalMIs) {
   unsigned TrueOp = 0;
   unsigned FalseOp = 0;
   bool Optimizable = false;
   SmallVector<MachineOperand, 4> Cond;
   if (TII->analyzeSelect(MI, Cond, TrueOp, FalseOp, Optimizable))
     return false;
   if (!Optimizable)
     return false;
   if (!TII->optimizeSelect(MI, LocalMIs))
     return false;
+  LLVM_DEBUG(dbgs() << "Deleting select: " << MI);
   MI.eraseFromParent();
   ++NumSelects;
   return true;
 }
 
 /// Check if a simpler conditional branch can be generated.
 bool PeepholeOptimizer::optimizeCondBranch(MachineInstr &MI) {
   return TII->optimizeCondBranch(MI);
bool PeepholeOptimizer::optimizeUncoalescableCopy(
   // The change is possible for all defs, do it.
   for (const RegSubRegPair &Def : RewritePairs) {
     // Rewrite the "copy" in a way the register coalescer understands.
     MachineInstr &NewCopy = rewriteSource(MI, Def, RewriteMap);
     LocalMIs.insert(&NewCopy);
   }
 
   // MI is now dead.
+  LLVM_DEBUG(dbgs() << "Deleting uncoalescable copy: " << MI);
   MI.eraseFromParent();
   ++NumUncoalescableCopies;
   return true;
 }
 
 /// Check whether MI is a candidate for folding into a later instruction.
 /// We only fold loads to virtual registers and the virtual register defined
 /// has a single user.
for (MachineBasicBlock::iterator MII = MBB.begin(), MIE = MBB.end();
         Changed = true;
         continue;
       }
 
       if (MI->isCopy() &&
           (foldRedundantCopy(*MI, CopySrcRegs, CopySrcMIs) ||
            foldRedundantNAPhysCopy(*MI, NAPhysToVirtMIs))) {
         LocalMIs.erase(MI);
+        LLVM_DEBUG(dbgs() << "Deleting redundant copy: " << *MI << "\n");
         MI->eraseFromParent();
         Changed = true;
         continue;
       }
 
       if (isMoveImmediate(*MI, ImmDefRegs, ImmDefMIs)) {
         SeenMoveImm = true;
       } else {

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

static bool canInstrSubstituteCmpInstr(MachineInstr *MI, MachineInstr *CmpInstr,
 
   AccessKind AccessToCheck = AK_Write;
   if (sForm(*MI) != MI->getOpcode())
     AccessToCheck = AK_All;
   if (areCFlagsAccessedBetweenInstrs(MI, CmpInstr, TRI, AccessToCheck))
     return false;
 
   UsedNZCV NZCVUsedAfterCmp;
-  for (auto I = std::next(CmpInstr->getIterator()),
-            E = CmpInstr->getParent()->instr_end();
-       I != E; ++I) {
-    const MachineInstr &Instr = *I;
+  for (const MachineInstr &Instr :
+       instructionsWithoutDebug(std::next(CmpInstr->getIterator()),
+                                CmpInstr->getParent()->instr_end())) {
     if (Instr.readsRegister(AArch64::NZCV, TRI)) {
       AArch64CC::CondCode CC = findCondCodeUsedByInstr(Instr);
       if (CC == AArch64CC::Invalid) // Unsupported conditional instruction
         return false;
       NZCVUsedAfterCmp |= getUsedNZCV(CC);
     }
 
     if (Instr.modifiesRegister(AArch64::NZCV, TRI))

llvm/test/CodeGen/AArch64/arm64-arm64-dead-def-elimination-flag.ll

-; RUN: llc -mtriple=arm64-apple-ios7.0.0 -aarch64-enable-dead-defs=false < %s | FileCheck %s
+; RUN: llc -debugify-and-strip-all-safe -mtriple=arm64-apple-ios7.0.0 -aarch64-enable-dead-defs=false < %s | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
 
 ; Function Attrs: nounwind ssp uwtable
 define i32 @test1() #0 {
   %tmp1 = alloca i8
   %tmp2 = icmp eq i8* %tmp1, null
   %tmp3 = zext i1 %tmp2 to i32
 
   ret i32 %tmp3
 
   ; CHECK-LABEL: test1
   ; CHECK: adds {{x[0-9]+}}, sp, #15
 }