[PATCH] D39869: [Inliner] Inline through indirect call sites having !callees metadata

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso updated this revision to Diff 129910.
mssimpso added a comment.

Addressed David's comments.

- Made `tryToPromote` visible to the indirect call promotion pass
- Added a check in `tryToPromote` that gives up if a call site has profile metadata to avoid conflicting heuristics.

David,

I hope I've at least addressed the spirit of you comments. First, I've moved `tryToPromote` to Transforms/Utils/CallPromotionUtils.cpp. IndirectCallPromotion.cpp already includes the necessary header, so the function is now visible there.

Second, regarding the inline cost callback, I made the profitability measure completely generic. So the client needs to provide `tryToPromote` with a function to determine if promotion is profitable, given an indirect call site and possible direct callee. I think it makes sense to implement it in this generic way if we leave the function in CallPromotionUtils.cpp. When called from within the inliner, the profitability measure is a wrapper around `shouldInline` that takes care of caching the inline cost to avoid recomputing it, etc. If we want to reuse the code in the indirect call promotion pass, we would need to define provide a similar profitability function.

I was unsure if you intended that I completely scrap all of the inliner changes, and make `tryToPromote` only available to the indirect call promotion pass. I didn't take your comment to mean this, but I could be wrong. Just in case, here are a few words in defense of the current patch.

In the current approach, where promotion is directed by the inliner, we limit promotion to the callees we know will definitely be inlined. So promotion is really just an intermediate step. We know the direct callee will be eliminated by inlining, so we're actually just inlining code through the indirect call. We also know the profitability measure we're trying to satisfy, inline cost, is accurate (but note that we need https://reviews.llvm.org/D39339 to get this right). Although a useful indicator, I think if we use inline cost outside the inliner, or at least outside a CallGraphSCCPass, it may end up being wrong in some cases. Suppose, for example, we use inline cost in a module pass that promotes an indirect call site to directly call function F. Later, the inliner may decide to inline a lot of code into F, making F a bad candidate for inlining into its caller. So the benefit we thought we were getting by promotion (the code simplification, measured by inline cost, due to inlining F into its caller) turns out to be nothing. This kind of situation is avoided by letting the inliner direct the promotion.

Thanks again!


Repository:
  rL LLVM

https://reviews.llvm.org/D39869

Files:
  include/llvm/Transforms/Utils/CallPromotionUtils.h
  lib/Transforms/IPO/Inliner.cpp
  lib/Transforms/Scalar/GVNSink.cpp
  lib/Transforms/Utils/CallPromotionUtils.cpp
  test/Other/new-pm-defaults.ll
  test/Other/new-pm-thinlto-defaults.ll
  test/Transforms/Inline/callees-metadata.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39869.129910.patch
Type: text/x-patch
Size: 31673 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180115/96cf7279/attachment.bin>