This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mingmingl on Jun 16 2023, 9:58 AM.

Details

Summary
  • The motivation is to expose tunable knobs to control the aggressiveness of inlines for different backend (e.g., machines with different icache size, and workload with different icache/itlb PMU counters). Tuning inline aggressiveness shows a small (~+0.3%) but stable improvement on workload/hardware that is more frontend bound.
  • Both multipliers could be overridden from command line.

Diff Detail

Event Timeline

mingmingl created this revision.Jun 16 2023, 9:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 9:58 AM
mingmingl requested review of this revision.Jun 16 2023, 9:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 9:58 AM
mingmingl added a subscriber: kazu.Jun 16 2023, 9:59 AM
mingmingl updated this revision to Diff 532971.Jun 20 2023, 9:56 AM
mingmingl retitled this revision from [Inliner]Use a hybrid of cost-benefit and cost-threshold analysis. to [NFC][Inliner] Introduce per-target savings multiplier and profitable multiplier to use a hybrid of cost-benefit and cost-threshold analysis..
mingmingl edited the summary of this revision. (Show Details)

Update patch to have per-target parameters. Tests WIP

kazu added inline comments.Jun 20 2023, 5:34 PM
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
72 ↗(On Diff #532971)

IIUC, with this patch, we stop honoring the command line option -inline-savings-multiplier. Is there any way you could retain the ability to specify the multiplier as a command line option so that we can easily experiment with different values without rebuilding the compiler? You could do something like:

if (InlineSavingsMultiplier.getNumOccurrences()) {
  // Honor the option.
  ...
} else {
  // Do whatever architecture-specific things we need to do.
  ...
}
llvm/lib/Analysis/InlineCost.cpp
920–924

IIUC, we are returning false if the cost-benefit ratio is below some threshold. Could you somehow mention that we are comparing the cost-benefit ratio against some threshold as a comment?

// Do not inline if the savings does not justify the cost of inlining.                                                  
// Specifically, we evaluate the following inequality:                                                                  
//                                                                                                                      
//  CycleSavings                PSI->getOrCompHotCountThreshold()                                                       
// -------------- <= --------------------------------------------------------                                           
//       Size        TTI.getInliningCostBenefitAnalysisProfitableMultiplier()

If you come with some concise language instead of nearly repeating the entire comment block, that's even better.

mingmingl marked an inline comment as done.
mingmingl retitled this revision from [NFC][Inliner] Introduce per-target savings multiplier and profitable multiplier to use a hybrid of cost-benefit and cost-threshold analysis. to [NFC][Inliner] Introduce another multiplier for cost benefit analysis and make multipliers overriddable in TargetTransformInfo..
mingmingl edited the summary of this revision. (Show Details)

Revive this NFC patch to expose tunable knobs for workloads/hardwares that could be more frontend bound.

Address feedback.

Let me know if you'd prefer to review this on Github PR or use Phabricator. I'm fine with both.

kazu added inline comments.Sep 25 2023, 12:52 PM
llvm/lib/Analysis/InlineCost.cpp
103

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(
107

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.
903–915

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.

928

Remove this empty inline for consistency with the block above.

932
// 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"?
llvm/test/Transforms/Inline/inline-cost-benefit-multiplier-override.ll
1 ↗(On Diff #557191)

Remove this empty line.

69 ↗(On Diff #557191)

Insert a new line at the end.

mingmingl updated this revision to Diff 557326.Sep 25 2023, 1:42 PM
mingmingl marked 7 inline comments as done.

Thanks for the suggestions around readability! Resolve comments.

llvm/test/Transforms/Inline/inline-cost-benefit-multiplier-override.ll
69 ↗(On Diff #557191)

Whoops. This No newline at end of file should be an inserted by the editor. I removed it.

kazu accepted this revision.Sep 28 2023, 1:30 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 28 2023, 1:30 PM

thanks for the reviews and suggestions! It's great to keep this in Phab (before reviews are fully on Github PR) ˙ᵕ˙

I'll do a sanity check of run, add a brief comment to explain the motivation (hardwares with much smaller icache and weaker frontend might need a per-target multiplier), and then land this.

I did a sanity check of this NFC change; the performance result is neutral (expected).