Page MenuHomePhabricator

[SimplifyCFG] Prevent SimplifyCFG from sinking indirect calls.
Needs ReviewPublic

Authored by jhuber6 on Sep 30 2021, 8:31 PM.

Details

Summary

This patch prevents the CFG simplification pass from sinking indirect
calls. Currently, the indirect calls will be combined into a single
indirect call after the arguments have been selected. This prevents the
indirect calls from being replaced with direct ones once the callee is
known through other optimizations.

An example of this is shown in https://godbolt.org/z/jTcfYnvYx where the
indirect calls cannot be replaced with direct calls once they are
inlined and the calls are known. This is generally more costly than
calling the function directly.

Diff Detail

Event Timeline

jhuber6 created this revision.Sep 30 2021, 8:31 PM
jhuber6 requested review of this revision.Sep 30 2021, 8:31 PM
jhuber6 edited the summary of this revision. (Show Details)Sep 30 2021, 8:33 PM
xgupta added a subscriber: xgupta.Oct 1 2021, 12:19 AM

It looks like what we need is a invoke of select between functions --> if-then-else

It looks like what we need is a invoke of select between functions --> if-then-else

So this should be a separate optimization run afterwards? Where do you think that optimization should live?

jdoerfert added inline comments.Oct 1 2021, 12:37 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2065–2066

CB has an isindirectcall member, IIRC.

It looks like what we need is a invoke of select between functions --> if-then-else

Looked at this problem again. There's already a way to promote indirect calls by versioning the call site, we could just add a pass that fully versions indirect calls that have callee metadata. If we do that for the code in the summary we'll get something like this https://godbolt.org/z/EP6n75far, then the versioning should be optimized out in cases like this with InstCombine, SimplifyCFG, and DCE leaving only conditional direct calls. Even in cases where this can't be optimized out, fully versioning out an indirect call whose callees are all known seems reasonable to me, maybe there's a reason we don't do this already that I'm not aware of. The downside is that fixing the code in the summary requires running the callee analysis after inlining, and then doing the versioning early enough that the other simplification passes will clean it up. I could make a patch for that if you think it's reasonable, but I'm not sure if it's worth the extra runtime in LLVM. The alternative is this patch which prevents this specific problem from showing up. What do you think @lebedev.ri?