This is an archive of the discontinued LLVM Phabricator instance.

Update successor after branch relaxation in ARM
AbandonedPublic

Authored by sabuasal on Oct 13 2017, 7:00 PM.

Details

Reviewers
rovka
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.

Diff Detail

Repository
rL LLVM

Event Timeline

sabuasal created this revision.Oct 13 2017, 7:00 PM

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

sabuasal updated this revision to Diff 119193.Oct 16 2017, 12:16 PM

Updated diff to include context.

rovka edited edge metadata.Oct 17 2017, 1:33 AM

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 updated this revision to Diff 119363.Oct 17 2017, 12:05 PM

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 added a comment.Oct 18 2017, 2:08 AM

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).

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

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

sabuasal abandoned this revision.Dec 7 2017, 12:23 PM