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

1718

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

1727

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

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 predecessor. 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();
   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() {
   bool MadeChange = false;
   for (unsigned i = 0, e = PushPopMIs.size(); i != e; ++i) {