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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Tried to be more clear below. If this still isn't making sense, maybe let's talk or use IRC?


================
Comment at: include/llvm/Analysis/InlineCost.h:121
@@ +120,3 @@
+  int DefaultThreshold;
+  /// Threshold to use for callees with inline hint.
+  int HintThreshold;
----------------
nit: I find it much easier to read with vertical whitespace between the values here.

================
Comment at: lib/Analysis/InlineCost.cpp:624
@@ +623,3 @@
+  // return min(A, B) if B is valid.
+  auto minIfValid = [](int A, Optional<int> B) {
+    return B ? std::min(A, B.getValue()) : A;
----------------
nit: We typically name local lambdas following the variable naming conventions even though they can be called.

================
Comment at: lib/Transforms/IPO/InlineSimple.cpp:79-81
@@ +78,5 @@
+
+  // Generate the parameters to tune the inline cost analysis based on
+  // commandline options.
+  InlineParams getInlineParams(int Threshold) {
+    InlineParams Params;
----------------
Sorry, I guess I wasn't clear before...

I'm specifically suggesting keeping all of this logic inside of InlineCost.cpp as how the *default* parameters get set.

If we need the ability to get custom parameters which still reflect the command line flags, there should be some API in InlineCost.h that the inliner queries to get the necessary params.


My goal here stems from two related principles:
- The tunable flags and the constants that interact are are fundamentally in the same unit domain should live in a single place (InlineCost.{h,cpp}).
- The inliner *passes* should have no real logic to compute thresholds, they should call well documented APIs provided by the cost layer.

A minor secondary goal -- the inliner shouldn't have to do extra work to get the defaults. Those should just be used. It is when an inliner pass wants to deviate from the defaults that it should take explicit steps and call explicit flags.


https://reviews.llvm.org/D22120