[PATCH] D153154: [NFC][Inliner] Introduce another multiplier for cost benefit analysis and make multipliers overriddable in TargetTransformInfo.

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:91
 
 static cl::opt<int> InlineSavingsMultiplier(
     "inline-savings-multiplier", cl::Hidden, cl::init(8),
----------------
I am the one that suggested:

```
if (InlineSavingsMultiplier.getNumOccurrences()) {
  // Honor the option.
  ...
} else {
  // Do whatever architecture-specific things we need to do.
  ...
}
```

below.  That said, with this approach, we do not use the default value of `InlineSavingsMultiplier` at all.  That is, changing the default value at the source code level (as opposed to the command-line level) has no effect.  This is very confusing.

I am not sure if there is a better way to juggle three things -- the compiler-wide default, the architecture-specific default, and the command-line override.  May I suggest a comment like this?

```
// We honor this option only when it is explicitly specified.
// The default value below isn't used at all.  If you wish to change it, update
// TargetTransformInfoImplBase::getInliningCostBenefitAnalysisSavingsMultiplier.
static cl::opt<int> InlineSavingsMultiplier(
```



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:95
 
+static cl::opt<int> InlineSavingsProfitableMultiplier(
+    "inline-savings-profitable-multiplier", cl::Hidden, cl::init(4),
----------------
Likewise, may I suggest a comment like this?

```
// We honor this option only when it is explicitly specified.
// The default value below isn't used at all.  If you wish to change it, update
// TargetTransformInfoImplBase::getInliningCostBenefitAnalysisProfitableMultiplier.
```



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:927-935
     // Return true if the savings justify the cost of inlining.  Specifically,
     // we evaluate the following inequality:
     //
     //  CycleSavings      PSI->getOrCompHotCountThreshold()
     // -------------- >= -----------------------------------
     //       Size              InlineSavingsMultiplier
     //
----------------
IIUC, you want to return `true` if the ratio of the cycle savings to the size is really high and `false` if it's really low.  If the ratio falls somewhere in the middle, then you want to fall back to the cost-based analysis.  If you are making this change, I would like to make this picture clear, especially given that it's not easy to read the code that tries to avoid divisions.

May I suggest replacing the comment above with something like this?

```
  // Let R be the ratio of CycleSavings to Size.  We accept the inlining
  // opportunity if R is really high and reject if R is really low.  If R is
  // somewhere in the middle, we fall back to the cost-based analysis.
  //
  // Specifically, we accept the inlining opportunity if:
  //
  //             PSI->getOrCompHotCountThreshold()
  // R > -------------------------------------------------
  //     getInliningCostBenefitAnalysisSavingsMultiplier()
  //
  // and reject the inlining opportunity if:
  //
  //                PSI->getOrCompHotCountThreshold()
  // R <= ----------------------------------------------------
  //      getInliningCostBenefitAnalysisProfitableMultiplier()
  //
  // Otherwise, we fall back to the cost-based analysis.
```

Sure, we would be repeating comments like "Otherwise, we fall back to" below, but IMHO it's much more important to get the high-level idea across.



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:948
+        getInliningCostBenefitAnalysisProfitableMultiplier();
+
+    if (LowerBoundCycleSavings.ult(Threshold))
----------------
Remove this empty inline for consistency with the block above.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:952
+
+    // Otherwise, fallback to cost-threshold analysis.
+    return std::nullopt;
----------------
```
// Otherwise, fall back to the cost-based analysis.
```

- Use "fall back" (with a space in between) as a verb.
- We are falling back to the specific analysis, so insert "the".
- "Cost-benefit analysis" is a proverbial phrase, but "cost-threshold analysis" isn't.  May I suggest "cost-based analysis" or "cost-only analysis"?



================
Comment at: llvm/test/Transforms/Inline/inline-cost-benefit-multiplier-override.ll:1
+
+; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -pass-remarks=inline -pass-remarks-missed=inline -inline-savings-multiplier=4 -inline-savings-profitable-multiplier=5 -S 2>&1| FileCheck %s
----------------
Remove this empty line.


================
Comment at: llvm/test/Transforms/Inline/inline-cost-benefit-multiplier-override.ll:69
+!18 = !{!"function_entry_count", i64 400}
\ No newline at end of file

----------------
Insert a new line at the end.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153154/new/

https://reviews.llvm.org/D153154