SystemZ: Avoid implicit iterator conversions, NFC

[dexonsmith]

Summary

Avoid implicit conversions from MachineInstrBundleIterator to
MachineInstr* in the SystemZ backend, mainly by preferring MachineInstr&
over MachineInstr* and using range-based for loops.

[dexonsmith]

+uweigand

[uweigand]

I'll defer to your expertise on that. Patch looks good to me.

I guess I'm not fully familiar with some of the C++ language details here. Would you mind explaining in more detail what the problem is specifically, and what we should be doing to avoid this issue in the future? Using something like "&*I" in the LDCleanup file wouldn't have naturally occurred to me ... :-)

[dexonsmith]

Committed in r275137, and somehow that didn't get included in Phab despite top-posting...

lib/Target/SystemZ/SystemZElimCompare.cpp

public:
   bool processBlock(MachineBasicBlock &MBB);
   bool runOnMachineFunction(MachineFunction &F) override;
   MachineFunctionProperties getRequiredProperties() const override {
     return MachineFunctionProperties().set(
         MachineFunctionProperties::Property::AllVRegsAllocated);
   }
 
 private:
-  Reference getRegReferences(MachineInstr *MI, unsigned Reg);
-  bool convertToBRCT(MachineInstr *MI, MachineInstr *Compare,
+  Reference getRegReferences(MachineInstr &MI, unsigned Reg);
+  bool convertToBRCT(MachineInstr &MI, MachineInstr &Compare,
                      SmallVectorImpl<MachineInstr *> &CCUsers);
-  bool convertToLoadAndTest(MachineInstr *MI);
-  bool adjustCCMasksForInstr(MachineInstr *MI, MachineInstr *Compare,
+  bool convertToLoadAndTest(MachineInstr &MI);
+  bool adjustCCMasksForInstr(MachineInstr &MI, MachineInstr &Compare,
                              SmallVectorImpl<MachineInstr *> &CCUsers);
-  bool optimizeCompareZero(MachineInstr *Compare,
+  bool optimizeCompareZero(MachineInstr &Compare,
                            SmallVectorImpl<MachineInstr *> &CCUsers);
-  bool fuseCompareOperations(MachineInstr *Compare,
+  bool fuseCompareOperations(MachineInstr &Compare,
                              SmallVectorImpl<MachineInstr *> &CCUsers);
 
   const SystemZInstrInfo *TII;
   const TargetRegisterInfo *TRI;
 };
 
 char SystemZElimCompare::ID = 0;
 } // end anonymous namespace
 
 FunctionPass *llvm::createSystemZElimComparePass(SystemZTargetMachine &TM) {
   return new SystemZElimCompare(TM);
 }
 
 // Return true if CC is live out of MBB.
 static bool isCCLiveOut(MachineBasicBlock &MBB) {
   for (auto SI = MBB.succ_begin(), SE = MBB.succ_end(); SI != SE; ++SI)
     if ((*SI)->isLiveIn(SystemZ::CC))
       return true;
   return false;
 }
 
 // Return true if any CC result of MI would reflect the value of Reg.
-static bool resultTests(MachineInstr *MI, unsigned Reg) {
-  if (MI->getNumOperands() > 0 &&
-      MI->getOperand(0).isReg() &&
-      MI->getOperand(0).isDef() &&
-      MI->getOperand(0).getReg() == Reg)
+static bool resultTests(MachineInstr &MI, unsigned Reg) {
+  if (MI.getNumOperands() > 0 && MI.getOperand(0).isReg() &&
+      MI.getOperand(0).isDef() && MI.getOperand(0).getReg() == Reg)
     return true;
 
-  switch (MI->getOpcode()) {
+  switch (MI.getOpcode()) {
   case SystemZ::LR:
   case SystemZ::LGR:
   case SystemZ::LGFR:
   case SystemZ::LTR:
   case SystemZ::LTGR:
   case SystemZ::LTGFR:
   case SystemZ::LER:
   case SystemZ::LDR:
   case SystemZ::LXR:
   case SystemZ::LTEBR:
   case SystemZ::LTDBR:
   case SystemZ::LTXBR:
-    if (MI->getOperand(1).getReg() == Reg)
+    if (MI.getOperand(1).getReg() == Reg)
       return true;
   }
 
   return false;
 }
 
 // Describe the references to Reg or any of its aliases in MI.
-Reference SystemZElimCompare::getRegReferences(MachineInstr *MI, unsigned Reg) {
+Reference SystemZElimCompare::getRegReferences(MachineInstr &MI, unsigned Reg) {
   Reference Ref;
-  for (unsigned I = 0, E = MI->getNumOperands(); I != E; ++I) {
-    const MachineOperand &MO = MI->getOperand(I);
+  for (unsigned I = 0, E = MI.getNumOperands(); I != E; ++I) {
+    const MachineOperand &MO = MI.getOperand(I);
     if (MO.isReg()) {
       if (unsigned MOReg = MO.getReg()) {
         if (TRI->regsOverlap(MOReg, Reg)) {
           if (MO.isUse())
             Ref.Use = true;
           else if (MO.isDef())
             Ref.Def = true;
         }
       }
     }
   }
   return Ref;
 }
 
 // Return true if this is a load and test which can be optimized the
 // same way as compare instruction.
-static bool isLoadAndTestAsCmp(MachineInstr *MI) {
+static bool isLoadAndTestAsCmp(MachineInstr &MI) {
   // If we during isel used a load-and-test as a compare with 0, the
   // def operand is dead.
-  return ((MI->getOpcode() == SystemZ::LTEBR ||
-           MI->getOpcode() == SystemZ::LTDBR ||
-           MI->getOpcode() == SystemZ::LTXBR) &&
-          MI->getOperand(0).isDead());
+  return (MI.getOpcode() == SystemZ::LTEBR ||
+          MI.getOpcode() == SystemZ::LTDBR ||
+          MI.getOpcode() == SystemZ::LTXBR) &&
+         MI.getOperand(0).isDead();
 }
 
 // Return the source register of Compare, which is the unknown value
 // being tested.
-static unsigned getCompareSourceReg(MachineInstr *Compare) {
+static unsigned getCompareSourceReg(MachineInstr &Compare) {
   unsigned reg = 0;
-  if (Compare->isCompare())
-    reg = Compare->getOperand(0).getReg();
+  if (Compare.isCompare())
+    reg = Compare.getOperand(0).getReg();
   else if (isLoadAndTestAsCmp(Compare))
-    reg = Compare->getOperand(1).getReg();
+    reg = Compare.getOperand(1).getReg();
   assert (reg);
 
   return reg;
 }
 
 // Compare compares the result of MI against zero.  If MI is an addition
 // of -1 and if CCUsers is a single branch on nonzero, eliminate the addition
 // and convert the branch to a BRCT(G).  Return true on success.
-bool
-SystemZElimCompare::convertToBRCT(MachineInstr *MI, MachineInstr *Compare,
-                                  SmallVectorImpl<MachineInstr *> &CCUsers) {
+bool SystemZElimCompare::convertToBRCT(
+    MachineInstr &MI, MachineInstr &Compare,
   // Check whether we have an addition of -1.
-  unsigned Opcode = MI->getOpcode();
+  unsigned Opcode = MI.getOpcode();
   unsigned BRCT;
   if (Opcode == SystemZ::AHI)
     BRCT = SystemZ::BRCT;
   else if (Opcode == SystemZ::AGHI)
     BRCT = SystemZ::BRCTG;
   else
     return false;
-  if (MI->getOperand(2).getImm() != -1)
+  if (MI.getOperand(2).getImm() != -1)
     return false;
 
   // Check whether we have a single JLH.
   if (CCUsers.size() != 1)
     return false;
   MachineInstr *Branch = CCUsers[0];
   if (Branch->getOpcode() != SystemZ::BRC ||
       Branch->getOperand(0).getImm() != SystemZ::CCMASK_ICMP ||
       Branch->getOperand(1).getImm() != SystemZ::CCMASK_CMP_NE)
     return false;
 
   // We already know that there are no references to the register between
   // MI and Compare.  Make sure that there are also no references between
   // Compare and Branch.
   unsigned SrcReg = getCompareSourceReg(Compare);
   MachineBasicBlock::iterator MBBI = Compare, MBBE = Branch;
   for (++MBBI; MBBI != MBBE; ++MBBI)
-    if (getRegReferences(MBBI, SrcReg))
+    if (getRegReferences(*MBBI, SrcReg))
       return false;
 
   // The transformation is OK.  Rebuild Branch as a BRCT(G).
   MachineOperand Target(Branch->getOperand(2));
   while (Branch->getNumOperands())
     Branch->RemoveOperand(0);
   Branch->setDesc(TII->get(BRCT));
   MachineInstrBuilder(*Branch->getParent()->getParent(), Branch)
-    .addOperand(MI->getOperand(0))
-    .addOperand(MI->getOperand(1))
+      .addOperand(MI.getOperand(0))
+      .addOperand(MI.getOperand(1))
       .addOperand(Target)
       .addReg(SystemZ::CC, RegState::ImplicitDefine | RegState::Dead);
-  MI->eraseFromParent();
+  MI.eraseFromParent();
   return true;
 }
 
 // If MI is a load instruction, try to convert it into a LOAD AND TEST.
 // Return true on success.
-bool SystemZElimCompare::convertToLoadAndTest(MachineInstr *MI) {
-  unsigned Opcode = TII->getLoadAndTest(MI->getOpcode());
+bool SystemZElimCompare::convertToLoadAndTest(MachineInstr &MI) {
+  unsigned Opcode = TII->getLoadAndTest(MI.getOpcode());
   if (!Opcode)
     return false;
 
-  MI->setDesc(TII->get(Opcode));
-  MachineInstrBuilder(*MI->getParent()->getParent(), MI)
+  MI.setDesc(TII->get(Opcode));
+  MachineInstrBuilder(*MI.getParent()->getParent(), MI)
       .addReg(SystemZ::CC, RegState::ImplicitDefine);
   return true;
 }
 
 // The CC users in CCUsers are testing the result of a comparison of some
 // value X against zero and we know that any CC value produced by MI
 // would also reflect the value of X.  Try to adjust CCUsers so that
 // they test the result of MI directly, returning true on success.
 // Leave everything unchanged on failure.
-bool SystemZElimCompare::
-adjustCCMasksForInstr(MachineInstr *MI, MachineInstr *Compare,
-                      SmallVectorImpl<MachineInstr *> &CCUsers) {
-  int Opcode = MI->getOpcode();
+bool SystemZElimCompare::adjustCCMasksForInstr(
+    MachineInstr &MI, MachineInstr &Compare,
+  int Opcode = MI.getOpcode();
   const MCInstrDesc &Desc = TII->get(Opcode);
   unsigned MIFlags = Desc.TSFlags;
 
   // See which compare-style condition codes are available.
   unsigned ReusableCCMask = SystemZII::getCompareZeroCCMask(MIFlags);
 
   // For unsigned comparisons with zero, only equality makes sense.
-  unsigned CompareFlags = Compare->getDesc().TSFlags;
+  unsigned CompareFlags = Compare.getDesc().TSFlags;
   if (CompareFlags & SystemZII::IsLogical)
     ReusableCCMask &= SystemZ::CCMASK_CMP_EQ;
 
   if (ReusableCCMask == 0)
     return false;
 
   unsigned CCValues = SystemZII::getCCValues(MIFlags);
   assert((ReusableCCMask & ~CCValues) == 0 && "Invalid CCValues");
for (unsigned I = 0, E = AlterMasks.size(); I != E; I += 2) {
     AlterMasks[I]->setImm(CCValues);
     unsigned CCMask = AlterMasks[I + 1]->getImm();
     if (CCMask & ~ReusableCCMask)
       AlterMasks[I + 1]->setImm((CCMask & ReusableCCMask) |
                                 (CCValues & ~ReusableCCMask));
   }
 
   // CC is now live after MI.
-  int CCDef = MI->findRegisterDefOperandIdx(SystemZ::CC, false, true, TRI);
+  int CCDef = MI.findRegisterDefOperandIdx(SystemZ::CC, false, true, TRI);
   assert(CCDef >= 0 && "Couldn't find CC set");
-  MI->getOperand(CCDef).setIsDead(false);
+  MI.getOperand(CCDef).setIsDead(false);
 
   // Clear any intervening kills of CC.
   MachineBasicBlock::iterator MBBI = MI, MBBE = Compare;
   for (++MBBI; MBBI != MBBE; ++MBBI)
     MBBI->clearRegisterKills(SystemZ::CC, TRI);
 
   return true;
 }
 
 // Return true if Compare is a comparison against zero.
-static bool isCompareZero(MachineInstr *Compare) {
-  switch (Compare->getOpcode()) {
+static bool isCompareZero(MachineInstr &Compare) {
+  switch (Compare.getOpcode()) {
   case SystemZ::LTEBRCompare:
   case SystemZ::LTDBRCompare:
   case SystemZ::LTXBRCompare:
     return true;
 
   default:
 
     if (isLoadAndTestAsCmp(Compare))
       return true;
 
-    return (Compare->getNumExplicitOperands() == 2 &&
-            Compare->getOperand(1).isImm() &&
-            Compare->getOperand(1).getImm() == 0);
+    return Compare.getNumExplicitOperands() == 2 &&
+           Compare.getOperand(1).isImm() && Compare.getOperand(1).getImm() == 0;
   }
 }
 
 // Try to optimize cases where comparison instruction Compare is testing
 // a value against zero.  Return true on success and if Compare should be
 // deleted as dead.  CCUsers is the list of instructions that use the CC
 // value produced by Compare.
-bool SystemZElimCompare::
-optimizeCompareZero(MachineInstr *Compare,
-                    SmallVectorImpl<MachineInstr *> &CCUsers) {
+bool SystemZElimCompare::optimizeCompareZero(
+    MachineInstr &Compare, SmallVectorImpl<MachineInstr *> &CCUsers) {
   if (!isCompareZero(Compare))
     return false;
 
   // Search back for CC results that are based on the first operand.
   unsigned SrcReg = getCompareSourceReg(Compare);
-  MachineBasicBlock &MBB = *Compare->getParent();
+  MachineBasicBlock &MBB = *Compare.getParent();
   MachineBasicBlock::iterator MBBI = Compare, MBBE = MBB.begin();
   Reference CCRefs;
   Reference SrcRefs;
   while (MBBI != MBBE) {
     --MBBI;
-    MachineInstr *MI = MBBI;
+    MachineInstr &MI = *MBBI;
     if (resultTests(MI, SrcReg)) {
       // Try to remove both MI and Compare by converting a branch to BRCT(G).
       // We don't care in this case whether CC is modified between MI and
       // Compare.
       if (!CCRefs.Use && !SrcRefs && convertToBRCT(MI, Compare, CCUsers)) {
         BranchOnCounts += 1;
         return true;
       }
while (MBBI != MBBE) {
     if (CCRefs.Use && CCRefs.Def)
       return false;
   }
   return false;
 }
 
 // Try to fuse comparison instruction Compare into a later branch.
 // Return true on success and if Compare is therefore redundant.
-bool SystemZElimCompare::
-fuseCompareOperations(MachineInstr *Compare,
-                      SmallVectorImpl<MachineInstr *> &CCUsers) {
+bool SystemZElimCompare::fuseCompareOperations(
+    MachineInstr &Compare, SmallVectorImpl<MachineInstr *> &CCUsers) {
   // See whether we have a single branch with which to fuse.
   if (CCUsers.size() != 1)
     return false;
   MachineInstr *Branch = CCUsers[0];
   SystemZII::FusedCompareType Type;
   switch (Branch->getOpcode()) {
   case SystemZ::BRC:
     Type = SystemZII::CompareAndBranch;
     break;
   case SystemZ::CondReturn:
     Type = SystemZII::CompareAndReturn;
     break;
   case SystemZ::CallBCR:
     Type = SystemZII::CompareAndSibcall;
     break;
   case SystemZ::CondTrap:
     Type = SystemZII::CompareAndTrap;
     break;
   default:
     return false;
   }
 
   // See whether we have a comparison that can be fused.
-  unsigned FusedOpcode = TII->getFusedCompare(Compare->getOpcode(),
-                                              Type, Compare);
+  unsigned FusedOpcode =
+      TII->getFusedCompare(Compare.getOpcode(), Type, &Compare);
   if (!FusedOpcode)
     return false;
 
   // Make sure that the operands are available at the branch.
-  unsigned SrcReg = Compare->getOperand(0).getReg();
-  unsigned SrcReg2 = (Compare->getOperand(1).isReg() ?
-                      Compare->getOperand(1).getReg() : 0);
+  unsigned SrcReg = Compare.getOperand(0).getReg();
+  unsigned SrcReg2 =
+      Compare.getOperand(1).isReg() ? Compare.getOperand(1).getReg() : 0;
   MachineBasicBlock::iterator MBBI = Compare, MBBE = Branch;
   for (++MBBI; MBBI != MBBE; ++MBBI)
     if (MBBI->modifiesRegister(SrcReg, TRI) ||
         (SrcReg2 && MBBI->modifiesRegister(SrcReg2, TRI)))
       return false;
 
   // Read the branch mask, target (if applicable), regmask (if applicable).
   MachineOperand CCMask(MBBI->getOperand(1));
if (Type == SystemZII::CompareAndBranch ||
       Type == SystemZII::CompareAndSibcall)
     Branch->RemoveOperand(2);
   Branch->RemoveOperand(1);
   Branch->RemoveOperand(0);
 
   // Rebuild Branch as a fused compare and branch.
   Branch->setDesc(TII->get(FusedOpcode));
   MachineInstrBuilder MIB(*Branch->getParent()->getParent(), Branch);
-  MIB.addOperand(Compare->getOperand(0))
-     .addOperand(Compare->getOperand(1))
+  MIB.addOperand(Compare.getOperand(0))
+      .addOperand(Compare.getOperand(1))
       .addOperand(CCMask);
 
   if (Type == SystemZII::CompareAndBranch) {
     // Only conditional branches define CC, as they may be converted back
     // to a non-fused branch because of a long displacement.  Conditional
     // returns don't have that problem.
     MIB.addOperand(Target)
        .addReg(SystemZ::CC, RegState::ImplicitDefine | RegState::Dead);
   }
bool SystemZElimCompare::processBlock(MachineBasicBlock &MBB) {
 
   // Walk backwards through the block looking for comparisons, recording
   // all CC users as we go.  The subroutines can delete Compare and
   // instructions before it.
   bool CompleteCCUsers = !isCCLiveOut(MBB);
   SmallVector<MachineInstr *, 4> CCUsers;
   MachineBasicBlock::iterator MBBI = MBB.end();
   while (MBBI != MBB.begin()) {
-    MachineInstr *MI = --MBBI;
-    if (CompleteCCUsers &&
-        (MI->isCompare() || isLoadAndTestAsCmp(MI)) &&
+    MachineInstr &MI = *--MBBI;
+    if (CompleteCCUsers && (MI.isCompare() || isLoadAndTestAsCmp(MI)) &&
         (optimizeCompareZero(MI, CCUsers) ||
          fuseCompareOperations(MI, CCUsers))) {
       ++MBBI;
-      MI->eraseFromParent();
+      MI.eraseFromParent();
       Changed = true;
       CCUsers.clear();
       continue;
     }
 
-    if (MI->definesRegister(SystemZ::CC)) {
+    if (MI.definesRegister(SystemZ::CC)) {
       CCUsers.clear();
       CompleteCCUsers = true;
     }
-    if (MI->readsRegister(SystemZ::CC) && CompleteCCUsers)
-      CCUsers.push_back(MI);
+    if (MI.readsRegister(SystemZ::CC) && CompleteCCUsers)
+      CCUsers.push_back(&MI);
   }
   return Changed;
 }
 
 bool SystemZElimCompare::runOnMachineFunction(MachineFunction &F) {
   if (skipFunction(*F.getFunction()))
     return false;
 

lib/Target/SystemZ/SystemZInstrInfo.cpp

 // each having the opcode given by NewOpcode.
 void SystemZInstrInfo::splitMove(MachineBasicBlock::iterator MI,
                                  unsigned NewOpcode) const {
   MachineBasicBlock *MBB = MI->getParent();
   MachineFunction &MF = *MBB->getParent();
 
   // Get two load or store instructions.  Use the original instruction for one
   // of them (arbitrarily the second here) and create a clone for the other.
-  MachineInstr *EarlierMI = MF.CloneMachineInstr(MI);
+  MachineInstr *EarlierMI = MF.CloneMachineInstr(&*MI);
   MBB->insert(MI, EarlierMI);
 
   // Set up the two 64-bit registers.
   MachineOperand &HighRegOp = EarlierMI->getOperand(0);
   MachineOperand &LowRegOp = MI->getOperand(0);
   HighRegOp.setReg(RI.getSubReg(HighRegOp.getReg(), SystemZ::subreg_h64));
   LowRegOp.setReg(RI.getSubReg(LowRegOp.getReg(), SystemZ::subreg_l64));
 
static bool removeIPMBasedCompare(MachineInstr &Compare, unsigned SrcReg,
   if (!IPM || IPM->getOpcode() != SystemZ::IPM)
     return false;
 
   // Check that there are no assignments to CC between the IPM and Compare,
   if (IPM->getParent() != Compare.getParent())
     return false;
   MachineBasicBlock::iterator MBBI = IPM, MBBE = Compare.getIterator();
   for (++MBBI; MBBI != MBBE; ++MBBI) {
-    MachineInstr *MI = MBBI;
-    if (MI->modifiesRegister(SystemZ::CC, TRI))
+    MachineInstr &MI = *MBBI;
+    if (MI.modifiesRegister(SystemZ::CC, TRI))
       return false;
   }
 
   Compare.eraseFromParent();
   if (LGFR)
     eraseIfDead(LGFR, MRI);
   eraseIfDead(RLL, MRI);
   eraseIfDead(SRL, MRI);

lib/Target/SystemZ/SystemZLDCleanup.cpp

bool SystemZLDCleanup::VisitNode(MachineDomTreeNode *Node,
   MachineBasicBlock *BB = Node->getBlock();
   bool Changed = false;
 
   // Traverse the current block.
   for (auto I = BB->begin(), E = BB->end(); I != E; ++I) {
     switch (I->getOpcode()) {
       case SystemZ::TLS_LDCALL:
         if (TLSBaseAddrReg)
-          I = ReplaceTLSCall(I, TLSBaseAddrReg);
+          I = ReplaceTLSCall(&*I, TLSBaseAddrReg);
         else
-          I = SetRegister(I, &TLSBaseAddrReg);
+          I = SetRegister(&*I, &TLSBaseAddrReg);
         Changed = true;
         break;
       default:
         break;
     }
   }
 
   // Visit the children of this block in the dominator tree.