[PATCH] D153154: [NFC][Inliner] Introduce another multiplier for cost benefit analysis and make multipliers overriddable in TargetTransformInfo.
Kazu Hirata via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 25 12:52:36 PDT 2023
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
More information about the llvm-commits
mailing list