Page MenuHomePhabricator

[NFCI-ish][SimplifyCFGPass] Rework and generalize `ret` block tail-merging

Authored by lebedev.ri on Jun 19 2021, 2:17 PM.



This changes the approach taken to tail-merge the blocks
to always create a new block instead of trying to reuse some block,
and generalizes it to support dealing not with just the ret in the future.

This effectively lifts the CallBr restriction, although this isn't really intentional.
That is the only non-NFC change here, i'm not sure if it's reasonable/feasible to temporarily retain it.

Other restrictions of the transform remain.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jun 19 2021, 2:17 PM
lebedev.ri requested review of this revision.Jun 19 2021, 2:17 PM

Split off addresstaken fixes.

lebedev.ri edited the summary of this revision. (Show Details)Jun 20 2021, 2:47 AM
rnk added inline comments.Jun 21 2021, 1:21 PM

Oh boy, that's fun. This is the first I've learned of this verifier requirement. In the future, maybe there could be a way to generalize looking for musttail and looking for this intrinsic. I don't see a good way right now.


See, tokens are totally straightforward compared to the deoptimize intrinsic. All the features that I've helped design are totally clear and straightforward. =P Or, at least I've tried to do my best, anyway.


Can this be Term->getPrevNonDebugInstruction?


Maybe Phis is a better name?


In the end, the new instructions should carry the merged source location from the original instructions. You probably want to use DILocation::getMergedLocations for that. This is also worth testing.

lebedev.ri marked 2 inline comments as done.Jun 21 2021, 1:24 PM

@rkn thank you for taking a look!


Yep, this is super mismodelled.
@llvm.experimental_deoptimize should simply implicitly be musttail,
and then everything would just work.

lebedev.ri marked 3 inline comments as done.

Hopefully address review notes.


I'd rather not change existing code just because we can, this code goes away in the very next patch.


How about NewOps?

Move code out of the loop since it looks cleaner that way.

rnk accepted this revision.Jun 22 2021, 3:15 PM



Sounds good

This revision is now accepted and ready to land.Jun 22 2021, 3:15 PM


Thank you for the review!