[PATCH] D22120: Move inline threshold related flags to InlineSimple.cpp

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.

LGTM with a bunch of nits addressed below. Thanks for bearing with me on the layering or structure of the code here, and thanks for the nice cleanup!


================
Comment at: include/llvm/Analysis/InlineCost.h:144
@@ +143,3 @@
+/// line options. If -inline-threshold option is not explicitly passed,
+/// \p Threshold used as the default threshold.
+InlineParams getInlineParams(int Threshold);
----------------
nit: s/used/is used/

================
Comment at: include/llvm/Analysis/InlineCost.h:149
@@ +148,3 @@
+/// line options. If -inline-threshold option is not explicitly passed,
+/// the default threshold is computed from \p OptLevel and \p SizeOptLevel.
+InlineParams getInlineParams(unsigned OptLevel, unsigned SizeOptLevel);
----------------
You might want to document what OptLevel and SizeOptLevel can be here. We've kind-of failed to do that for a long time sadly, but I think you can at least document the requirements that the internal code places upon them even if we want to incrementally improve this later.

================
Comment at: include/llvm/Analysis/InlineCost.h:164-166
@@ -114,5 +163,5 @@
 InlineCost
-getInlineCost(CallSite CS, int DefaultThreshold, TargetTransformInfo &CalleeTTI,
+getInlineCost(CallSite CS, TargetTransformInfo &CalleeTTI,
               std::function<AssumptionCache &(Function &)> &GetAssumptionCache,
-              ProfileSummaryInfo *PSI);
+              ProfileSummaryInfo *PSI, const InlineParams &Params);
 
----------------
nit: I would put the Params parameter in the same position as DefaultThreshold was previously here and below.

================
Comment at: lib/Analysis/InlineCost.cpp:959
@@ -969,3 +958,3 @@
   // out. Pretend to inline the function, with a custom threshold.
-  CallAnalyzer CA(TTI, GetAssumptionCache, PSI, *F,
-                  InlineConstants::IndirectCallThreshold, CS);
+  InlineParams IndirectCallParams(Params);
+  IndirectCallParams.DefaultThreshold = InlineConstants::IndirectCallThreshold;
----------------
As you're not calling a constructor I would use `=` for initializing. Also, auto seems appropriate here. So:

  auto IndirectCallParams = Params;

================
Comment at: lib/Analysis/InlineCost.cpp:1540-1545
@@ +1539,8 @@
+
+// APIs to create InlineParams based on command line flags and/or other
+// parameters. Generally, users of inline cost analysis obtain an InlineParams
+// object through one of these interfaces and pass it to getInlineCost above.
+// Some specialized versions of inliner (such as the pre-inliner) might not
+// want the command line flags to influence the threshold, in which case they
+// might have their own logic to compute InlineParams.
+
----------------
This seems like it might be a better fit merged into the comments for the InlineParams class? Certainly it seems like documentation for users rather than implementation documentation.


https://reviews.llvm.org/D22120