[Codegen] Remove dead flags on Physical Defs in machine cse

[dmgreen]

Summary

We may leave behind incorrect dead flags on instructions that are CSE'd. Make
sure we remove the dead flags on these instructions to prevent other incorrect
code motion.

[dmgreen]

ping :)

[efriedma]

It's odd nobody has run into this before, but LGTM with minor nits.

lib/CodeGen/MachineCSE.cpp

279 ↗

(On Diff #186443)

You could use llvm::enumerate here; maybe more readable. Otherwise, please use an "I < E" loop (http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop) .

test/CodeGen/Thumb/machine-cse-deadreg.mir

1 ↗

(On Diff #186443)

-verify-machine-instrs

llvm/trunk/lib/CodeGen/MachineCSE.cpp

namespace {
 
   private:
     using AllocatorTy = RecyclingAllocator<BumpPtrAllocator,
                             ScopedHashTableVal<MachineInstr *, unsigned>>;
     using ScopedHTType =
         ScopedHashTable<MachineInstr *, unsigned, MachineInstrExpressionTrait,
                         AllocatorTy>;
     using ScopeType = ScopedHTType::ScopeTy;
+    using PhysDefVector = SmallVector<std::pair<unsigned, unsigned>, 2>;
 
     unsigned LookAheadLimit = 0;
     DenseMap<MachineBasicBlock *, ScopeType *> ScopeMap;
     ScopedHTType VNT;
     SmallVector<MachineInstr *, 64> Exps;
     unsigned CurrVN = 0;
 
     bool PerformTrivialCopyPropagation(MachineInstr *MI,
                                        MachineBasicBlock *MBB);
     bool isPhysDefTriviallyDead(unsigned Reg,
                                 MachineBasicBlock::const_iterator I,
                                 MachineBasicBlock::const_iterator E) const;
     bool hasLivePhysRegDefUses(const MachineInstr *MI,
                                const MachineBasicBlock *MBB,
-                               SmallSet<unsigned,8> &PhysRefs,
-                               SmallVectorImpl<unsigned> &PhysDefs,
-                               bool &PhysUseDef) const;
+                               SmallSet<unsigned, 8> &PhysRefs,
+                               PhysDefVector &PhysDefs, bool &PhysUseDef) const;
     bool PhysRegDefsReach(MachineInstr *CSMI, MachineInstr *MI,
-                          SmallSet<unsigned,8> &PhysRefs,
-                          SmallVectorImpl<unsigned> &PhysDefs,
-                          bool &NonLocal) const;
+                          SmallSet<unsigned, 8> &PhysRefs,
+                          PhysDefVector &PhysDefs, bool &NonLocal) const;
     bool isCSECandidate(MachineInstr *MI);
     bool isProfitableToCSE(unsigned CSReg, unsigned Reg,
                            MachineInstr *CSMI, MachineInstr *MI);
     void EnterScope(MachineBasicBlock *MBB);
     void ExitScope(MachineBasicBlock *MBB);
     bool ProcessBlock(MachineBasicBlock *MBB);
     void ExitScopeIfDone(MachineDomTreeNode *Node,
                          DenseMap<MachineDomTreeNode*, unsigned> &OpenChildren);

 }
 
 /// hasLivePhysRegDefUses - Return true if the specified instruction read/write
 /// physical registers (except for dead defs of physical registers). It also
 /// returns the physical register def by reference if it's the only one and the
 /// instruction does not uses a physical register.
 bool MachineCSE::hasLivePhysRegDefUses(const MachineInstr *MI,
                                        const MachineBasicBlock *MBB,
-                                       SmallSet<unsigned,8> &PhysRefs,
-                                       SmallVectorImpl<unsigned> &PhysDefs,
-                                       bool &PhysUseDef) const{
+                                       SmallSet<unsigned, 8> &PhysRefs,
+                                       PhysDefVector &PhysDefs,
+                                       bool &PhysUseDef) const {
   // First, add all uses to PhysRefs.
   for (const MachineOperand &MO : MI->operands()) {
     if (!MO.isReg() || MO.isDef())
       continue;
     unsigned Reg = MO.getReg();
     if (!Reg)
       continue;
     if (TargetRegisterInfo::isVirtualRegister(Reg))
       continue;
     // Reading either caller preserved or constant physregs is ok.
     if (!isCallerPreservedOrConstPhysReg(Reg, *MI->getMF(), *TRI))
       for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
         PhysRefs.insert(*AI);
   }
 
   // Next, collect all defs into PhysDefs.  If any is already in PhysRefs
   // (which currently contains only uses), set the PhysUseDef flag.
   PhysUseDef = false;
   MachineBasicBlock::const_iterator I = MI; I = std::next(I);
-  for (const MachineOperand &MO : MI->operands()) {
+  for (const auto &MOP : llvm::enumerate(MI->operands())) {
+    const MachineOperand &MO = MOP.value();
     if (!MO.isReg() || !MO.isDef())
       continue;
     unsigned Reg = MO.getReg();
     if (!Reg)
       continue;
     if (TargetRegisterInfo::isVirtualRegister(Reg))
       continue;
     // Check against PhysRefs even if the def is "dead".
     if (PhysRefs.count(Reg))
       PhysUseDef = true;
     // If the def is dead, it's ok. But the def may not marked "dead". That's
     // common since this pass is run before livevariables. We can scan
     // forward a few instructions and check if it is obviously dead.
     if (!MO.isDead() && !isPhysDefTriviallyDead(Reg, I, MBB->end()))
-      PhysDefs.push_back(Reg);
+      PhysDefs.push_back(std::make_pair(MOP.index(), Reg));
   }
 
   // Finally, add all defs to PhysRefs as well.
   for (unsigned i = 0, e = PhysDefs.size(); i != e; ++i)
-    for (MCRegAliasIterator AI(PhysDefs[i], TRI, true); AI.isValid(); ++AI)
+    for (MCRegAliasIterator AI(PhysDefs[i].second, TRI, true); AI.isValid();
+         ++AI)
       PhysRefs.insert(*AI);
 
   return !PhysRefs.empty();
 }
 
 bool MachineCSE::PhysRegDefsReach(MachineInstr *CSMI, MachineInstr *MI,
-                                  SmallSet<unsigned,8> &PhysRefs,
-                                  SmallVectorImpl<unsigned> &PhysDefs,
+                                  SmallSet<unsigned, 8> &PhysRefs,
+                                  PhysDefVector &PhysDefs,
                                   bool &NonLocal) const {
   // For now conservatively returns false if the common subexpression is
   // not in the same basic block as the given instruction. The only exception
   // is if the common subexpression is in the sole predecessor block.
   const MachineBasicBlock *MBB = MI->getParent();
   const MachineBasicBlock *CSMBB = CSMI->getParent();
 
   bool CrossMBB = false;
   if (CSMBB != MBB) {
     if (MBB->pred_size() != 1 || *MBB->pred_begin() != CSMBB)
       return false;
 
     for (unsigned i = 0, e = PhysDefs.size(); i != e; ++i) {
-      if (MRI->isAllocatable(PhysDefs[i]) || MRI->isReserved(PhysDefs[i]))
+      if (MRI->isAllocatable(PhysDefs[i].second) ||
+          MRI->isReserved(PhysDefs[i].second))
         // Avoid extending live range of physical registers if they are
         //allocatable or reserved.
         return false;
     }
     CrossMBB = true;
   }
   MachineBasicBlock::const_iterator I = CSMI; I = std::next(I);
   MachineBasicBlock::const_iterator E = MI;
if (!FoundCSE && MI->isCommutable()) {
       }
     }
 
     // If the instruction defines physical registers and the values *may* be
     // used, then it's not safe to replace it with a common subexpression.
     // It's also not safe if the instruction uses physical registers.
     bool CrossMBBPhysDef = false;
     SmallSet<unsigned, 8> PhysRefs;
-    SmallVector<unsigned, 2> PhysDefs;
+    PhysDefVector PhysDefs;
     bool PhysUseDef = false;
     if (FoundCSE && hasLivePhysRegDefUses(MI, MBB, PhysRefs,
                                           PhysDefs, PhysUseDef)) {
       FoundCSE = false;
 
       // ... Unless the CS is local or is in the sole predecessor block
       // and it also defines the physical register which is not clobbered
       // in between and the physical register uses were not clobbered.
if (DoCSE) {
         MRI->replaceRegWith(OldReg, NewReg);
         MRI->clearKillFlags(NewReg);
       }
 
       // Go through implicit defs of CSMI and MI, if a def is not dead at MI,
       // we should make sure it is not dead at CSMI.
       for (unsigned ImplicitDefToUpdate : ImplicitDefsToUpdate)
         CSMI->getOperand(ImplicitDefToUpdate).setIsDead(false);
+      for (auto PhysDef : PhysDefs)
+        if (!MI->getOperand(PhysDef.first).isDead())
+          CSMI->getOperand(PhysDef.first).setIsDead(false);
 
       // Go through implicit defs of CSMI and MI, and clear the kill flags on
       // their uses in all the instructions between CSMI and MI.
       // We might have made some of the kill flags redundant, consider:
       //   subs  ... implicit-def %nzcv    <- CSMI
       //   csinc ... implicit killed %nzcv <- this kill flag isn't valid anymore
       //   subs  ... implicit-def %nzcv    <- MI, to be eliminated
       //   csinc ... implicit killed %nzcv
if (DoCSE) {
         for (auto ImplicitDef : ImplicitDefs)
           MRI->clearKillFlags(ImplicitDef);
       }
 
       if (CrossMBBPhysDef) {
         // Add physical register defs now coming in from a predecessor to MBB
         // livein list.
         while (!PhysDefs.empty()) {
-          unsigned LiveIn = PhysDefs.pop_back_val();
-          if (!MBB->isLiveIn(LiveIn))
-            MBB->addLiveIn(LiveIn);
+          auto LiveIn = PhysDefs.pop_back_val();
+          if (!MBB->isLiveIn(LiveIn.second))
+            MBB->addLiveIn(LiveIn.second);
         }
         ++NumCrossBBCSEs;
       }
 
       MI->eraseFromParent();
       ++NumCSEs;
       if (!PhysRefs.empty())
         ++NumPhysCSEs;

llvm/trunk/test/CodeGen/Thumb/machine-cse-deadreg.mir

+# RUN: llc -mtriple thumbv6m-arm-none-eabi -run-pass=machine-cse -verify-machineinstrs -o - %s | FileCheck %s
+
+--- |
+  target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "thumbv6m-arm-none-eabi"
+
+  define i32 @funca(i64 %l1) {
+  entry:
+    %l2 = icmp ult i64 %l1, -177673816660004267
+    %l3 = add i64 %l1, 401100203
+    %rem.i = select i1 %l2, i64 %l1, i64 %l3
+    %conv = trunc i64 %rem.i to i32
+    ret i32 %conv
+  }
+
+  define i32 @funcb(i64 %l1) { ret i32 0 }
+
+...
+---
+name:            funca
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0', virtual-reg: '%0' }
+  - { reg: '$r1', virtual-reg: '%1' }
+constants:
+  - id:              0
+    value:           i32 401100203
+    alignment:       4
+    isTargetSpecific: false
+  - id:              1
+    value:           i32 41367909
+    alignment:       4
+    isTargetSpecific: false
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x40000000), %bb.2(0x40000000)
+    liveins: $r0, $r1
+
+    %1:tgpr = COPY $r1
+    %0:tgpr = COPY $r0
+    %2:tgpr = tLDRpci %const.0, 14, $noreg :: (load 4 from constant-pool)
+    %3:tgpr, dead $cpsr = tADDrr %0, %2, 14, $noreg
+    %4:tgpr = tLDRpci %const.1, 14, $noreg :: (load 4 from constant-pool)
+    %5:tgpr, $cpsr = tADDrr %0, %2, 14, $noreg
+    %6:tgpr, $cpsr = tADC %1, killed %4, 14, $noreg, implicit $cpsr
+    tBcc %bb.2, 3, $cpsr
+
+  bb.1.entry:
+    successors: %bb.2(0x80000000)
+
+  bb.2.entry:
+    %7:tgpr = PHI %3, %bb.1, %0, %bb.0
+    $r0 = COPY %7
+    tBX_RET 14, $noreg, implicit $r0
+
+# CHECK-LABEL: name: funca
+# cpsr def must not be dead
+# CHECK: %3:tgpr, $cpsr = tADDrr %0, %2
+# CHECK-NOT: %5:tgpr, $cpsr = tADDrr %0, %2
+
+...
+---
+name:            funcb
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0', virtual-reg: '%0' }
+  - { reg: '$r1', virtual-reg: '%1' }
+constants:
+  - id:              0
+    value:           i32 401100203
+    alignment:       4
+    isTargetSpecific: false
+  - id:              1
+    value:           i32 41367909
+    alignment:       4
+    isTargetSpecific: false
+body:             |
+  bb.0:
+    successors: %bb.1(0x40000000), %bb.2(0x40000000)
+    liveins: $r0, $r1
+
+    %1:tgpr = COPY $r1
+    %0:tgpr = COPY $r0
+    %2:tgpr = tLDRpci %const.0, 14, $noreg :: (load 4 from constant-pool)
+    %3:tgpr, dead $cpsr = tADDrr %0, %2, 14, $noreg
+    %4:tgpr = tLDRpci %const.1, 14, $noreg :: (load 4 from constant-pool)
+    %5:tgpr, dead $cpsr = tADDrr %0, %2, 14, $noreg
+    %6:tgpr, $cpsr = tADDrr %1, killed %4, 14, $noreg
+    tBcc %bb.2, 3, $cpsr
+
+  bb.1:
+    successors: %bb.2(0x80000000)
+
+  bb.2:
+    %7:tgpr = PHI %3, %bb.1, %0, %bb.0
+    $r0 = COPY %7
+    tBX_RET 14, $noreg, implicit $r0
+
+# CHECK-LABEL: name: funcb
+# cpsr def should be dead
+# CHECK: %3:tgpr, dead $cpsr = tADDrr %0, %2
+# CHECK-NOT: %5:tgpr, dead $cpsr = tADDrr %0, %2
+...