Update successor after branch relaxation in ARM

[sabuasal]

Summary

While relaxing branches in ARM constant Island we might need to split
the basic block the branch instruction belongs to.

The splitBlockBeforeInstr API transfers the successors to the new block
including the destination block of the machine instruction. Later on
a branch is added from the Basic Block to the destination block
without adding the destination block as a successor. This patch fixes
that.

[javed.absar]

Context is missing. You may want to generate diff using -U999

[sabuasal]

Updated diff to include context.

[rovka]

This looks correct, but is there anything removing DestBB from the newly created block's successors if necessary?

lib/Target/ARM/ARMConstantIslandPass.cpp

1721

s/predecessor/successor/

[sabuasal]

You are right. I updated the change, if we did indeed split the block the patch now deletes the DestBB from successor list of the new block.
I only cared about not having the Dest BB as a successor because it might cause scheduling issues and\or labels not being emitted, having extra successor wont.

[rovka]

In D38918#899993, @sabuasal wrote:

I only cared about not having the Dest BB as a successor because it might cause scheduling issues and\or labels not being emitted, having extra successor wont.

Do you have an example that you could add as a MIR test for this patch? (If you get stuck parsing MIR constant pools, it's ok to pass IR through llc and just -stop-after this pass)

lib/Target/ARM/ARMConstantIslandPass.cpp

1730

I think you should make sure this is true (i.e. check that DestBB is not used by any of the other terminators in SplitBlock).

[sabuasal]

In D38918#900603, @rovka wrote:

In D38918#899993, @sabuasal wrote:

I only cared about not having the Dest BB as a successor because it might cause scheduling issues and\or labels not being emitted, having extra successor wont.

Do you have an example that you could add as a MIR test for this patch? (If you get stuck parsing MIR constant pools, it's ok to pass IR through llc and just -stop-after this pass)

Unfortunately I don't. I found this in a n out of tree that has modification to ARMConstant Island. What I can do is look at existing test cases and see if I can show the problem in the successor information.

I think you should make sure this is true (i.e. check that DestBB is not used by any of the other terminators in SplitBlock).

Hmm, ok

[sabuasal]

this was fixed in 318148 (but checks for removing the successor list are not added)

lib/Target/ARM/ARMConstantIslandPass.cpp

ARMConstantIslands::fixupConditionalBr(ImmBranch &Br) {
 
   // 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();
   MachineInstr *BMI = &MBB->back();
   bool NeedSplit = (BMI != MI) || !BBHasFallthrough(MBB);
 
+  // Pointer to the block split from MBB if needed.
+  MachineBasicBlock *SplitBlock = nullptr;
+
   ++NumCBrFixed;
   if (BMI != MI) {
     if (std::next(MachineBasicBlock::iterator(MI)) == std::prev(MBB->end()) &&
         BMI->getOpcode() == Br.UncondBr) {
       // 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 (isBBInRange(MI, NewDest, Br.MaxDisp)) {
         DEBUG(dbgs() << "  Invert Bcc condition and swap its destination with "
                      << *BMI);
         BMI->getOperand(0).setMBB(DestBB);
         MI->getOperand(0).setMBB(NewDest);
         MI->getOperand(1).setImm(CC);
         return true;
       }
     }
   }
 
   if (NeedSplit) {
-    splitBlockBeforeInstr(MI);
+    SplitBlock = splitBlockBeforeInstr(MI);
     // No need for the branch to the next block. We're adding an unconditional
     // branch to the destination.
     int delta = TII->getInstSizeInBytes(MBB->back());
     BBInfo[MBB->getNumber()].Size -= delta;
     MBB->back().eraseFromParent();
     // BBInfo[SplitBB].Offset is wrong temporarily, fixed below
   }
   MachineBasicBlock *NextBB = &*++MBB->getIterator();
BuildMI(MBB, DebugLoc(), TII->get(Br.UncondBr))
         .addMBB(DestBB)
         .add(predOps(ARMCC::AL));
   else
     BuildMI(MBB, DebugLoc(), TII->get(Br.UncondBr)).addMBB(DestBB);
   BBInfo[MBB->getNumber()].Size += TII->getInstSizeInBytes(MBB->back());
   unsigned MaxDisp = getUnconditionalBrDisp(Br.UncondBr);
   ImmBranches.push_back(ImmBranch(&MBB->back(), MaxDisp, false, Br.UncondBr));
 
+  if (NeedSplit) {
+    // When we split the MBB we transferred all its successors to the newly
+    // created block so the DestBB is no longer marked as a successor. Add
+    // it back here.
+    MBB->addSuccessor(DestBB);
+  }
+
   // Remove the old conditional branch.  It may or may not still be in MBB.
   BBInfo[MI->getParent()->getNumber()].Size -= TII->getInstSizeInBytes(*MI);
   MI->eraseFromParent();
+  // If we split the Block MI belongs to the new one. After deleting MI
+  // DestBB is no longer a successor.
+  if (SplitBlock)
+    SplitBlock->removeSuccessor(DestBB);
+
   adjustBBOffsetsAfter(MBB);
   return true;
 }
 
 /// undoLRSpillRestore - Remove Thumb push / pop instructions that only spills
 /// LR / restores LR to pc. FIXME: This is done here because it's only possible
 /// to do this if tBfar is not used.
 bool ARMConstantIslands::undoLRSpillRestore() {