[PATCH] D15701: Refactor inline costs analysis by removing the InlineCostAnalysis class

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

This LGTM, feel free to submit with the argument naming tweak below.

However, this naming tweak raises an interesting question for me. This should *not* be addressed in this patch, but it might be a good thing to put on your queue.

We are passing the *callee* TTI into the inline cost analysis. That doesn't make a lot of sense to me. We're inlining into the *caller*. If there is something *incompatible* about the callee, we should refuse to inline it. If they are compatible, the *caller's* TTI should become dominant. As an example, if we have one function which is marked as optimized for size, but we inline it into code that is not optimized for size, I would expect the inlined body to *not* be optimized for size. I would generally expect the TTI of the call site to determine the cost analysis for inlining because after inlining, the callee is gone. Does that make sense? That will be a non-trivial functional change, so you'll want to benchmark it etc before making it.

Relatedly, I know you're looking heavily at getting our nested call site analysis to be accurate. You should make sure we're considering the possibility of incompatible function attributes that might prevent inlining entirely. I don't think we currently model that correctly, in that the cost analysis skips it but then the inliner itself considers it.


================
Comment at: include/llvm/Analysis/InlineCost.h:112
@@ -125,15 +111,3 @@
   /// inlining the callsite. It is an expensive, heavyweight call.
-  InlineCost getInlineCost(CallSite CS, int Threshold);
-
-  /// \brief Get an InlineCost with the callee explicitly specified.
-  /// This allows you to calculate the cost of inlining a function via a
-  /// pointer. This behaves exactly as the version with no explicit callee
-  /// parameter in all other respects.
-  //
-  //  Note: This is used by out-of-tree passes, please do not remove without
-  //  adding a replacement API.
-  InlineCost getInlineCost(CallSite CS, Function *Callee, int Threshold);
-
-  /// \brief Minimal filter to detect invalid constructs for inlining.
-  bool isInlineViable(Function &Callee);
-};
+InlineCost getInlineCost(CallSite CS, int Threshold, TargetTransformInfo &TTI,
+                         AssumptionCacheTracker *ACT);
----------------
I would clarify that this is currently the callee's TTI, maybe just by naming it "CalleeTTI".


Repository:
  rL LLVM

http://reviews.llvm.org/D15701