AArch64: Fix end iterator dereference

[arsenm]

Summary

Not all blocks have terminators. I'm not sure how this wasn't crashing before.

[john.brawn]

It looks like this currently happens to not crash because ilist::end() points to a sentinel value which can be validly accessed. Relying on that implementation detail seems like a bad idea though, so this looks like a good idea.

lib/Target/AArch64/AArch64BranchRelaxation.cpp

265–266

Is there a particular reason you're changing this to take a reference instead of a pointer (and also getDestBlock below)? If you're doing it to emphasize "this pointer cannot be null" then it would make sense to do the same thing everywhere (i.e. in fixupConditionalBranch, invertBccCondition, etc.).

473

Making a reference only to then take the address of it when calling fixupConditionalBranch seems a little odd. I'd just stick with a pointer.

[arsenm]

lib/Target/AArch64/AArch64BranchRelaxation.cpp

473

I was trying to avoid &*I which is gross

[arsenm]

Use reference parameters everywhere

[arsenm]

ping

[rovka]

LGTM.

[arsenm]

r277427

lib/Target/AArch64/AArch64BranchRelaxation.cpp

class AArch64BranchRelaxation : public MachineFunctionPass {
 
   SmallVector<BasicBlockInfo, 16> BlockInfo;
 
   MachineFunction *MF;
   const AArch64InstrInfo *TII;
 
   bool relaxBranchInstructions();
   void scanFunction();
-  MachineBasicBlock *splitBlockBeforeInstr(MachineInstr *MI);
+  MachineBasicBlock *splitBlockBeforeInstr(MachineInstr &MI);
   void adjustBlockOffsets(MachineBasicBlock &MBB);
-  bool isBlockInRange(MachineInstr *MI, MachineBasicBlock *BB, unsigned Disp);
-  bool fixupConditionalBranch(MachineInstr *MI);
+  bool isBlockInRange(MachineInstr &MI, MachineBasicBlock &BB, unsigned Disp);
+  bool fixupConditionalBranch(MachineInstr &MI);
   void computeBlockSize(const MachineBasicBlock &MBB);
-  unsigned getInstrOffset(MachineInstr *MI) const;
+  unsigned getInstrOffset(MachineInstr &MI) const;
   void dumpBBs();
   void verify();
 
 public:
   static char ID;
   AArch64BranchRelaxation() : MachineFunctionPass(ID) {
     initializeAArch64BranchRelaxationPass(*PassRegistry::getPassRegistry());
   }
void AArch64BranchRelaxation::computeBlockSize(const MachineBasicBlock &MBB) {
   for (const MachineInstr &MI : MBB)
     Size += TII->GetInstSizeInBytes(MI);
   BlockInfo[MBB.getNumber()].Size = Size;
 }
 
 /// getInstrOffset - Return the current offset of the specified machine
 /// instruction from the start of the function.  This offset changes as stuff is
 /// moved around inside the function.
-unsigned AArch64BranchRelaxation::getInstrOffset(MachineInstr *MI) const {
-  MachineBasicBlock *MBB = MI->getParent();
+unsigned AArch64BranchRelaxation::getInstrOffset(MachineInstr &MI) const {
+  MachineBasicBlock *MBB = MI.getParent();
 
   // The offset is composed of two things: the sum of the sizes of all MBB's
   // before this instruction's block, and the offset from the start of the block
   // it is in.
   unsigned Offset = BlockInfo[MBB->getNumber()].Offset;
 
   // Sum instructions before MI in MBB.
-  for (MachineBasicBlock::iterator I = MBB->begin(); &*I != MI; ++I) {
+  for (MachineBasicBlock::iterator I = MBB->begin(); &*I != &MI; ++I) {
     assert(I != MBB->end() && "Didn't find MI in its own basic block?");
     Offset += TII->GetInstSizeInBytes(*I);
   }
+
   return Offset;
 }
 
 void AArch64BranchRelaxation::adjustBlockOffsets(MachineBasicBlock &Start) {
   unsigned PrevNum = Start.getNumber();
   for (auto &MBB : make_range(MachineFunction::iterator(Start), MF->end())) {
     unsigned Num = MBB.getNumber();
     if (!Num) // block zero is never changed from offset zero.
       continue;
     // Get the offset and known bits at the end of the layout predecessor.
     // Include the alignment of the current block.
     unsigned LogAlign = MBB.getAlignment();
     BlockInfo[Num].Offset = BlockInfo[PrevNum].postOffset(LogAlign);
     PrevNum = Num;
   }
 }
 
 /// Split the basic block containing MI into two blocks, which are joined by
 /// an unconditional branch.  Update data structures and renumber blocks to
 /// account for this change and returns the newly created block.
 /// NOTE: Successor list of the original BB is out of date after this function,
 /// and must be updated by the caller! Other transforms follow using this
 /// utility function, so no point updating now rather than waiting.
 MachineBasicBlock *
-AArch64BranchRelaxation::splitBlockBeforeInstr(MachineInstr *MI) {
-  MachineBasicBlock *OrigBB = MI->getParent();
+AArch64BranchRelaxation::splitBlockBeforeInstr(MachineInstr &MI) {
+  MachineBasicBlock *OrigBB = MI.getParent();
 
   // Create a new MBB for the code after the OrigBB.
   MachineBasicBlock *NewBB =
       MF->CreateMachineBasicBlock(OrigBB->getBasicBlock());
   MF->insert(++OrigBB->getIterator(), NewBB);
 
   // Splice the instructions starting with MI over to NewBB.
-  NewBB->splice(NewBB->end(), OrigBB, MI, OrigBB->end());
+  NewBB->splice(NewBB->end(), OrigBB, MI.getIterator(), OrigBB->end());
 
   // Add an unconditional branch from OrigBB to NewBB.
   // Note the new unconditional branch is not being recorded.
   // There doesn't seem to be meaningful DebugInfo available; this doesn't
   // correspond to anything in the source.
   BuildMI(OrigBB, DebugLoc(), TII->get(AArch64::B)).addMBB(NewBB);
 
   // Insert an entry into BlockInfo to align it properly with the block numbers.
AArch64BranchRelaxation::splitBlockBeforeInstr(MachineInstr &MI) {
   adjustBlockOffsets(*OrigBB);
 
   ++NumSplit;
 
   return NewBB;
 }
 
 /// isBlockInRange - Returns true if the distance between specific MI and
 /// specific BB can fit in MI's displacement field.
-bool AArch64BranchRelaxation::isBlockInRange(MachineInstr *MI,
-                                             MachineBasicBlock *DestBB,
+bool AArch64BranchRelaxation::isBlockInRange(MachineInstr &MI,
+                                             MachineBasicBlock &DestBB,
                                              unsigned Bits) {
   unsigned MaxOffs = ((1 << (Bits - 1)) - 1) << 2;
   unsigned BrOffset = getInstrOffset(MI);
-  unsigned DestOffset = BlockInfo[DestBB->getNumber()].Offset;
+  unsigned DestOffset = BlockInfo[DestBB.getNumber()].Offset;
 
-  DEBUG(dbgs() << "Branch of destination BB#" << DestBB->getNumber()
-               << " from BB#" << MI->getParent()->getNumber()
-               << " max delta=" << MaxOffs << " from " << getInstrOffset(MI)
+  DEBUG(dbgs() << "Branch of destination BB#" << DestBB.getNumber()
+               << " from BB#" << MI.getParent()->getNumber()
+               << " max delta=" << MaxOffs << " from " << BrOffset
                << " to " << DestOffset << " offset "
-               << int(DestOffset - BrOffset) << "\t" << *MI);
+               << static_cast<int>(DestOffset - BrOffset) << '\t' << MI);
 
   // Branch before the Dest.
   if (BrOffset <= DestOffset)
     return (DestOffset - BrOffset <= MaxOffs);
   return (BrOffset - DestOffset <= MaxOffs);
 }
 
 static bool isConditionalBranch(unsigned Opc) {
   switch (Opc) {
   default:
     return false;
   case AArch64::TBZW:
   case AArch64::TBNZW:
   case AArch64::TBZX:
   case AArch64::TBNZX:
   case AArch64::CBZW:
   case AArch64::CBNZW:
   case AArch64::CBZX:
   case AArch64::CBNZX:
   case AArch64::Bcc:
     return true;
   }
 }
 
-static MachineBasicBlock *getDestBlock(MachineInstr *MI) {
-  switch (MI->getOpcode()) {
+static MachineBasicBlock *getDestBlock(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
   default:
     llvm_unreachable("unexpected opcode!");
   case AArch64::TBZW:
   case AArch64::TBNZW:
   case AArch64::TBZX:
   case AArch64::TBNZX:
-    return MI->getOperand(2).getMBB();
+    return MI.getOperand(2).getMBB();
   case AArch64::CBZW:
   case AArch64::CBNZW:
   case AArch64::CBZX:
   case AArch64::CBNZX:
   case AArch64::Bcc:
-    return MI->getOperand(1).getMBB();
+    return MI.getOperand(1).getMBB();
   }
 }
 
 static unsigned getOppositeConditionOpcode(unsigned Opc) {
   switch (Opc) {
   default:
     llvm_unreachable("unexpected opcode!");
   case AArch64::TBNZW:   return AArch64::TBZW;
static unsigned getBranchDisplacementBits(unsigned Opc) {
   case AArch64::CBNZX:
   case AArch64::CBZX:
     return CBZDisplacementBits;
   case AArch64::Bcc:
     return BCCDisplacementBits;
   }
 }
 
-static inline void invertBccCondition(MachineInstr *MI) {
-  assert(MI->getOpcode() == AArch64::Bcc && "Unexpected opcode!");
-  AArch64CC::CondCode CC = (AArch64CC::CondCode)MI->getOperand(0).getImm();
+static inline void invertBccCondition(MachineInstr &MI) {
+  assert(MI.getOpcode() == AArch64::Bcc && "Unexpected opcode!");
+  AArch64CC::CondCode CC = (AArch64CC::CondCode)MI.getOperand(0).getImm();
   CC = AArch64CC::getInvertedCondCode(CC);
-  MI->getOperand(0).setImm((int64_t)CC);
+  MI.getOperand(0).setImm((int64_t)CC);
 }
 
 /// fixupConditionalBranch - Fix up a conditional branch whose destination is
 /// too far away to fit in its displacement field. It is converted to an inverse
 /// conditional branch + an unconditional branch to the destination.
-bool AArch64BranchRelaxation::fixupConditionalBranch(MachineInstr *MI) {
+bool AArch64BranchRelaxation::fixupConditionalBranch(MachineInstr &MI) {
   MachineBasicBlock *DestBB = getDestBlock(MI);
 
   // Add an unconditional branch to the destination and invert the branch
   // condition to jump over it:
   // tbz L1
   // =>
   // tbnz L2
   // b   L1
   // L2:
 
   // If the branch is at the end of its MBB and that has a fall-through block,
   // direct the updated conditional branch to the fall-through block. Otherwise,
   // split the MBB before the next instruction.
-  MachineBasicBlock *MBB = MI->getParent();
+  MachineBasicBlock *MBB = MI.getParent();
   MachineInstr *BMI = &MBB->back();
-  bool NeedSplit = (BMI != MI) || !BBHasFallthrough(MBB);
+  bool NeedSplit = (BMI != &MI) || !BBHasFallthrough(MBB);
 
-  if (BMI != MI) {
+  if (BMI != &MI) {
     if (std::next(MachineBasicBlock::iterator(MI)) ==
             std::prev(MBB->getLastNonDebugInstr()) &&
         BMI->getOpcode() == AArch64::B) {
       // Last MI in the BB is an unconditional branch. Can we simply invert the
       // condition and swap destinations:
       // beq L1
       // b   L2
       // =>
       // bne L2
       // b   L1
       MachineBasicBlock *NewDest = BMI->getOperand(0).getMBB();
-      if (isBlockInRange(MI, NewDest,
-                         getBranchDisplacementBits(MI->getOpcode()))) {
+      if (isBlockInRange(MI, *NewDest,
+                         getBranchDisplacementBits(MI.getOpcode()))) {
         DEBUG(dbgs() << "  Invert condition and swap its destination with "
                      << *BMI);
         BMI->getOperand(0).setMBB(DestBB);
-        unsigned OpNum = (MI->getOpcode() == AArch64::TBZW ||
-                          MI->getOpcode() == AArch64::TBNZW ||
-                          MI->getOpcode() == AArch64::TBZX ||
-                          MI->getOpcode() == AArch64::TBNZX)
+        unsigned OpNum = (MI.getOpcode() == AArch64::TBZW ||
+                          MI.getOpcode() == AArch64::TBNZW ||
+                          MI.getOpcode() == AArch64::TBZX ||
+                          MI.getOpcode() == AArch64::TBNZX)
                              ? 2
                              : 1;
-        MI->getOperand(OpNum).setMBB(NewDest);
-        MI->setDesc(TII->get(getOppositeConditionOpcode(MI->getOpcode())));
-        if (MI->getOpcode() == AArch64::Bcc)
+        MI.getOperand(OpNum).setMBB(NewDest);
+        MI.setDesc(TII->get(getOppositeConditionOpcode(MI.getOpcode())));
+        if (MI.getOpcode() == AArch64::Bcc)
           invertBccCondition(MI);
         return true;
       }
     }
   }
 
   if (NeedSplit) {
     // Analyze the branch so we know how to update the successor lists.
if (NeedSplit) {
     NewBB->addSuccessor(FBB);
   }
   MachineBasicBlock *NextBB = &*std::next(MachineFunction::iterator(MBB));
 
   DEBUG(dbgs() << "  Insert B to BB#" << DestBB->getNumber()
                << ", invert condition and change dest. to BB#"
                << NextBB->getNumber() << "\n");
 
+  unsigned OppositeCondOpc = getOppositeConditionOpcode(MI.getOpcode());
   // Insert a new conditional branch and a new unconditional branch.
-  MachineInstrBuilder MIB = BuildMI(
-      MBB, DebugLoc(), TII->get(getOppositeConditionOpcode(MI->getOpcode())))
-                                .addOperand(MI->getOperand(0));
-  if (MI->getOpcode() == AArch64::TBZW || MI->getOpcode() == AArch64::TBNZW ||
-      MI->getOpcode() == AArch64::TBZX || MI->getOpcode() == AArch64::TBNZX)
-    MIB.addOperand(MI->getOperand(1));
-  if (MI->getOpcode() == AArch64::Bcc)
-    invertBccCondition(MIB);
+  MachineInstrBuilder MIB = BuildMI(MBB, DebugLoc(), TII->get(OppositeCondOpc))
+    .addOperand(MI.getOperand(0));
+
+  if (MI.getOpcode() == AArch64::TBZW || MI.getOpcode() == AArch64::TBNZW ||
+      MI.getOpcode() == AArch64::TBZX || MI.getOpcode() == AArch64::TBNZX)
+    MIB.addOperand(MI.getOperand(1));
+  if (MI.getOpcode() == AArch64::Bcc)
+    invertBccCondition(*MIB);
   MIB.addMBB(NextBB);
   BlockInfo[MBB->getNumber()].Size += TII->GetInstSizeInBytes(MBB->back());
   BuildMI(MBB, DebugLoc(), TII->get(AArch64::B)).addMBB(DestBB);
   BlockInfo[MBB->getNumber()].Size += TII->GetInstSizeInBytes(MBB->back());
 
   // Remove the old conditional branch.  It may or may not still be in MBB.
-  BlockInfo[MI->getParent()->getNumber()].Size -= TII->GetInstSizeInBytes(*MI);
-  MI->eraseFromParent();
+  BlockInfo[MI.getParent()->getNumber()].Size -= TII->GetInstSizeInBytes(MI);
+  MI.eraseFromParent();
 
   // Finally, keep the block offsets up to date.
   adjustBlockOffsets(*MBB);
   return true;
 }
 
 bool AArch64BranchRelaxation::relaxBranchInstructions() {
   bool Changed = false;
   // Relaxing branches involves creating new basic blocks, so re-eval
   // end() for termination.
   for (MachineFunction::iterator I = MF->begin(); I != MF->end(); ++I) {
     MachineBasicBlock &MBB = *I;
-    MachineInstr &MI = *MBB.getFirstTerminator();
+    MachineBasicBlock::iterator J = MBB.getFirstTerminator();
+    if (J == MBB.end())
+      continue;
+
+    MachineInstr &MI = *J;
     if (isConditionalBranch(MI.getOpcode()) &&
-        !isBlockInRange(&MI, getDestBlock(&MI),
+        !isBlockInRange(MI, *getDestBlock(MI),
                         getBranchDisplacementBits(MI.getOpcode()))) {
-      fixupConditionalBranch(&MI);
+      fixupConditionalBranch(MI);
       ++NumRelaxed;
       Changed = true;
     }
   }
   return Changed;
 }
 
 bool AArch64BranchRelaxation::runOnMachineFunction(MachineFunction &mf) {