Page MenuHomePhabricator

[CloneFunction] Support BB == PredBB in DuplicateInstructionsInSplit.

Authored by fhahn on Feb 27 2018, 8:19 AM.



In case PredBB == BB and StopAt == BB's terminator, StopAt != &*BI will
fail, because BB's terminator instruction gets replaced.

By using BB.getTerminator() we get the current terminator which we can use to compare.

Diff Detail


Event Timeline

fhahn created this revision.Feb 27 2018, 8:19 AM

Just want to make sure if this is the only case where StopAt is changed in the middle ?

junbuml added inline comments.Feb 27 2018, 11:04 AM
816 ↗(On Diff #136082)

Since StopAt is Instruction*, this function cannot clone the terminator itself. We can clone up to one instruction before the terminator. Curious if this is what this function was initially intended.

fhahn added a comment.Feb 28 2018, 1:37 AM

Just want to make sure if this is the only case where StopAt is changed in the middle ?

AFAIK this is the only case due to how SplitEdge behaves when BB == PredBB. The other instructions should not be moved around while splitting.

816 ↗(On Diff #136082)

Not sure in which cases it would make sense to clone the terminator, as the New BB needs it's own terminator to branch to BB.

anna added inline comments.Mar 1 2018, 7:53 AM
814 ↗(On Diff #136082)

Basically, StopAt was pointing to the old terminator of BB before it was replaced. SplitEdge does not update BB's terminator. So, where exactly, is the update happening for BB's terminator now?

If PredBB == BB, then BB's terminator should be updated from "br i1 <cond> BB, otherBB" to "br i1 <cond> NewBB, otherBB".

fhahn added inline comments.Mar 1 2018, 9:28 AM
814 ↗(On Diff #136082)

The case that caused the crash was PredBB == BB and BB has an unconditional branch as a terminator. Then we split BB at the terminator, which replaces the terminator of BB (relevant line in SplitEdge;326450$273 , BasicBlock::splitBasicBlock, which is used by SplitBlock;326450$372)

anna accepted this revision.Mar 5 2018, 7:58 AM


814 ↗(On Diff #136082)

yes, that makes sense.

This revision is now accepted and ready to land.Mar 5 2018, 7:58 AM
This revision was automatically updated to reflect the committed changes.