[InlineCost] Disable cost-benefit when sample based PGO is used

[helloguo]

#66457 makes InlineCost to use cost-benefit by default, which causes 0.4-0.5% performance regression on multiple internal workloads. See discussions #66457. This pull request reverts it.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Xiangyang (Mark) Guo (helloguo)

Changes

#66457 makes InlineCost to use cost-benefit by default, which causes 0.4-0.5% performance regression on multiple internal workloads. See discussions #66457. This pull request reverts it.

---

Full diff: https://github.com/llvm/llvm-project/pull/86626.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/InlineCost.cpp (+1-1)
• (modified) llvm/test/Transforms/SampleProfile/remarks-hotness.ll (+2-2)

diff --git a/llvm/lib/Analysis/InlineCost.cpp b/llvm/lib/Analysis/InlineCost.cpp
index e55eaa55f8e947..c75460f44c1d9f 100644
--- a/llvm/lib/Analysis/InlineCost.cpp
+++ b/llvm/lib/Analysis/InlineCost.cpp
@@ -800,7 +800,7 @@ class InlineCostCallAnalyzer final : public CallAnalyzer {
         return false;
     } else {
       // Otherwise, require instrumentation profile.
-      if (!(PSI->hasInstrumentationProfile() || PSI->hasSampleProfile()))
+      if (!PSI->hasInstrumentationProfile())
         return false;
     }
 
diff --git a/llvm/test/Transforms/SampleProfile/remarks-hotness.ll b/llvm/test/Transforms/SampleProfile/remarks-hotness.ll
index b90b21e9e3c582..36fb3c58181703 100644
--- a/llvm/test/Transforms/SampleProfile/remarks-hotness.ll
+++ b/llvm/test/Transforms/SampleProfile/remarks-hotness.ll
@@ -24,7 +24,7 @@
 
 ; YAML-PASS:      --- !Passed
 ; YAML-PASS-NEXT: Pass:            inline
-; YAML-PASS-NEXT: Name:            AlwaysInline
+; YAML-PASS-NEXT: Name:            Inlined
 ; YAML-PASS-NEXT: DebugLoc:        { File: remarks-hotness.cpp, Line: 10, Column: 10 }
 ; YAML-PASS-NEXT: Function:        _Z7caller1v
 ; YAML-PASS-NEXT: Hotness:         401
@@ -36,7 +36,7 @@
 ; YAML-MISS-NEXT: Function:        _Z7caller2v
 ; YAML-MISS-NEXT: Hotness:         2
 
-; CHECK-RPASS: '_Z7callee1v' inlined into '_Z7caller1v' with (cost=always): benefit over cost at callsite _Z7caller1v:1:10; (hotness: 401)
+; CHECK-RPASS: '_Z7callee1v' inlined into '_Z7caller1v' with (cost=-30, threshold=4500) at callsite _Z7caller1v:1:10; (hotness: 401)
 ; CHECK-RPASS-NOT: '_Z7callee2v' not inlined into '_Z7caller2v' because it should never be inlined (cost=never): noinline function attribute (hotness: 2)
 
 ; ModuleID = 'remarks-hotness.cpp'

[kazutakahirata]

I think reverting the patch is probably the best. IIUC:

• @HaohaiWen's internal loads perform better with the cost-benefit analysis enabled for sample PGO.
• @helloguo's internal loads perform worse with the cost-benefit analysis enabled for sample PGO.
• I don't have numbers our internal loads right now although I do remember that most of the inlining opportunities are taken by the sample loader inliner llvm/lib/Transforms/IPO/SampleProfile.cpp.

In a case like this, I am inclined to ask those who do see benefits to manually turn on the cost-benefit analysis (i.e. -mllvm -inline-enable-cost-benefit-analysis). IMHO, features turned on by default should benefit broad audience.

Thoughts?

[williamweixiao]

I think reverting the patch is probably the best. IIUC:

• @HaohaiWen's internal loads perform better with the cost-benefit analysis enabled for sample PGO.
• @helloguo's internal loads perform worse with the cost-benefit analysis enabled for sample PGO.
• I don't have numbers our internal loads right now although I do remember that most of the inlining opportunities are taken by the sample loader inliner llvm/lib/Transforms/IPO/SampleProfile.cpp.

In a case like this, I am inclined to ask those who do see benefits to manually turn on the cost-benefit analysis (i.e. -mllvm -inline-enable-cost-benefit-analysis). IMHO, features turned on by default should benefit broad audience.

Thoughts?

I'm just curious about why cost-benefit analysis degrades performance for sample PGO while not for instrumentation PGO. Have you analyzed the root cause of performance regression on your multiple internal workloads? Is it because sample PGO profile quality is worse than that of instrumentation profile or any other reasons? I understand that reverting is the easiest way to fix internal performance issue but it will also limit sample PGO to expose more optimization opportunities.

[helloguo]

Is it because sample PGO profile quality is worse than that of instrumentation profile or any other reasons?

The profile quality could be one of reasons. In addition, cost-benefit subtracts ColdSize based on the assumption that machine function splitting could handle the cold code later. MFS may need fine tuning for sample based pgo.

but it will also limit sample PGO to expose more optimization opportunities

Users can always manually turn on the cost-benefit analysis with flags. I guess the discussion here is more about if we want to turn on cost-benefit by default. Changing the default probably requires more data to support the decision.

[williamweixiao]

The profile quality could be one of reasons. In addition, cost-benefit subtracts ColdSize based on the assumption that
machine function splitting could handle the cold code later. MFS may need fine tuning for sample based pgo.

I'm a little confused. The MFS issue is specific for sample PGO? The instrumentation PGO doesn't need the MFS tuning?

[helloguo]

The MFS issue is specific for sample PGO? The instrumentation PGO doesn't need the MFS tuning?

Based on the discussion https://reviews.llvm.org/D98213 MFS hasn't been tuned for AutoFDO.

[WenleiHe]

We can keep the discussion going. But until this shows universally better performance, let's back it out for now.

[HaohaiWen]

Hi, @WenleiHe,
I think MFS should be okay for both IPGO/SPGO. It just split cold BB into cold section based on BPI. Those logic is same for IPGO/SPGO. The difference is BPI quality.
We just found 2 issues of poor BPI quality due to optimization.

1. We found MFS split some HotBB to cold section due to wrong BPI. See [InstCombine] Update BranchProbabilityAnalysis cache result #86470. But I believe this issue should also exist for IPGO. There're may be another places with same mistake.
2. Some branch_weight update algorithm may not consider sampling is not absolutely accurate so its algorithm may wrap unsigned leading to big mistake for branch weight: [LoopRotate] Set loop back edge weight to not less than exit weight #86496

Both issues are not directly due to flaw in MFS. I think what we need to tune is poor BPI quality. not MFS.
Is it acceptable for you to measure the performance after landing of those two fixes and then decide to land this patch or not?

[WenleiHe]

@HaohaiWen Thanks for working on profile maintenance. I don't think the patches you mentioned are going to change the results meaningfully. Is there a particular reason you want this to be default on? As others mentioned, you can just turn it on as needed.

I'd also like to share some general feedbacks: 1) landing default changes like the original patch generally has a high bar. 2) the way it was done, without perf numbers, and without stakeholder review before landing is not how things are usually done.

[HaohaiWen]

@HaohaiWen Thanks for working on profile maintenance. I don't think the patches you mentioned are going to change the results meaningfully. Is there a particular reason you want this to be default on? As others mentioned, you can just turn it on as needed.

We'd like to turn it on since we have seen gains on our internal workloads. We hope it can be always beneficial for all workloads. Unfortunately it caused regression in your workloads. I agree to disable it for SPGO currently to workaround your regression. In long term, I hope we can try to tune and turn it on after fixing real root cause of this regression.

I'd also like to share some general feedbacks: 1) landing default changes like the original patch generally has a high bar. 2) the way it was done, without perf numbers, and without stakeholder review before landing is not how things are usually done.

Thanks for feedbacks.

[github-actions]

@helloguo Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!