Page MenuHomePhabricator

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

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

Details

Summary

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
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
117

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.

128–129

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.

128–132

Can this be Term->getPrevNonDebugInstruction?

149

Maybe Phis is a better name?

234

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!

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
117

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.

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
128–132

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

149

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

lgtm

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
128–132

Sounds good

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

lgtm

Thank you for the review!