This is an archive of the discontinued LLVM Phabricator instance.

[CallSiteSplitting] Pass list of (BB, Conditions) pairs to splitCallSite.
ClosedPublic

Authored by fhahn on Jan 9 2018, 5:26 AM.

Details

Summary

This removes some duplication from splitCallSite and makes it easier to
add additional code dealing with each predecessor. It also allows us to
split for more than 2 predecessors, although that is not enabled for
now.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jan 9 2018, 5:26 AM

Sorry for the late response. Just minor comments.

lib/Transforms/Scalar/CallSiteSplitting.cpp
112 ↗(On Diff #129065)

Why don't we use 2 instead of 3?

152 ↗(On Diff #129065)

Why not use ConditionsTy here ?

282–283 ↗(On Diff #129065)

It appears that this loop can be merged into the above loop (line 251), and then we can remove SplitInfo.

fhahn updated this revision to Diff 129946.Jan 16 2018, 6:39 AM

Thanks Jun! I've removed the extra loop dealing with the call PHI node

fhahn marked an inline comment as done.Jan 16 2018, 6:43 AM
fhahn added inline comments.
lib/Transforms/Scalar/CallSiteSplitting.cpp
112 ↗(On Diff #129065)

We potentially record more than 2 conditions now I think and I thought 3 might be better suited to avoid dynamic allocations. I am happy to change it to 2 if you prefer that.

152 ↗(On Diff #129065)

This function does not require knowing the size of the SmallVector, so I thought there is no need to make the type more restrictive. I am happy to change it if you prefer that

junbuml accepted this revision.Jan 16 2018, 8:43 AM

LGTM !

lib/Transforms/Scalar/CallSiteSplitting.cpp
112 ↗(On Diff #129065)

I'm okay with 3, but I don't think this change increase the possibility of recording more than 2 conditions through single predecessors.

152 ↗(On Diff #129065)

As it's used only with ConditionsTy tightly in our current implementation, I may prefer to be consistent with ConditionsTy for now.

282–283 ↗(On Diff #129065)

Thanks for handling this.

This revision is now accepted and ready to land.Jan 16 2018, 8:43 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jan 16 2018, 2:16 PM

Thanks again Jun! I've committed, addressing your 2 suggestions.