[TTI][AMDGPU] Allow targets to adjust LastCallToStaticBonus via getInliningLastCallToStaticBonus

[shiltian]

Currently we will not be able to inline a large function even if it only has one live use because the inline cost is still very high after applying LastCallToStaticBonus, which is a constant. This could significantly impact the performance because CSR spill is very expensive.

This PR adds a new function getInliningLastCallToStaticBonus to TTI to allow targets to customize this value.

Fixes SWDEV-471398.

[shiltian]

• [TTI][AMDGPU] Allow targets to adjust LastCallToStaticBonus via getInliningLastCallToStaticBonus  #111311 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @shiltian and the rest of your teammates on Graphite

[jmmartinez]

but the cost is still very close to the threshold: cost=14010, threshold=170775.

It's a 10x difference. Looks pretty safe to me. What am I missing?

TBH I do not see much of an easy alternative to what you're doing.

---

To test, instead of using big functions, you could pass -inline-threshold=0 -debug-only=inline-cost and check that the inline threshold increases between a function that is called once and another that is called twice.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-amdgpu

Author: Shilei Tian (shiltian)

Changes

Currently we will not inline a large function even if it only has one live use.
This could significantly impact the performance because CSR spill is very
expensive. The goal of this PR is trying to force the inlining if there is only
one live use by adjusting the inlining threshold, which is a configurable
number. The default value is 15000, which borrows from
InlineConstants::LastCallToStaticBonus. I'm not sure if this is a good number,
and if this is the right way to do that. After making this change, the callee in
my local test case can finally be inlined, but the cost is still very close to
the threshold: cost=14010, threshold=170775.

Speaking of the test, how are we gonna test this? Do we want to include a giant
IR file?

Fixes SWDEV-471398.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp (+10)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
index d348166c2d9a04..debc3db78974ad 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
@@ -75,6 +75,10 @@ static cl::opt<size_t> InlineMaxBB(
     cl::desc("Maximum number of BBs allowed in a function after inlining"
              " (compile time constraint)"));
 
+static cl::opt<unsigned> InlineThresholdOneLiveUse(
+    "amdgpu-inline-threshold-one-live-use", cl::Hidden, cl::init(15000),
+    cl::desc("Threshold added when the callee only has one live use"));
+
 static bool dependsOnLocalPhi(const Loop *L, const Value *Cond,
                               unsigned Depth = 0) {
   const Instruction *I = dyn_cast<Instruction>(Cond);
@@ -1307,6 +1311,12 @@ unsigned GCNTTIImpl::adjustInliningThreshold(const CallBase *CB) const {
   unsigned AllocaSize = getCallArgsTotalAllocaSize(CB, DL);
   if (AllocaSize > 0)
     Threshold += ArgAllocaCost;
+
+  // Increase the threshold if it is the only call to a local function.
+  Function *Callee = CB->getCalledFunction();
+  if (Callee->hasLocalLinkage() && Callee->hasOneLiveUse())
+    Threshold += InlineThresholdOneLiveUse;
+
   return Threshold;
 }
 

[arsenm]

Needs test. Isn't there a generic control for this already?

[nikic]

I'm a bit confused here. This already exists in the form of a cost bonus in the generic inlining cost model.

Is the point here that you want to effectively double the bonus by both applying it as a bonus in the generic model and as a threshold adjustment in AMDGPU TTI?

[shiltian]

I'm a bit confused here. This already exists in the form of a cost bonus in the generic inlining cost model.

Is the point here that you want to effectively double the bonus by both applying it as a bonus in the generic model and as a threshold adjustment in AMDGPU TTI?

Yes. IIUC, the generic model doesn't seem to have a target dependent approach to adjust the bonus. The default value is not sufficient and we want to inline it regardless.

[arsenm]

Yes. IIUC, the generic model doesn't seem to have a target dependent approach to adjust the bonus. The default value is not sufficient and we want to inline it regardless.

I'd rather avoid splitting the logic for this. Where is the default handling?

[shiltian]

Where is the default handling?

llvm-project/llvm/lib/Analysis/InlineCost.cpp

Line 2032 in 8a9e9a8

Cost -= LastCallToStaticBonus;

[shiltian]

It's a 10x difference. Looks pretty safe to me. What am I missing?

I missed one figure Lol.

[nikic]

I'm a bit confused here. This already exists in the form of a cost bonus in the generic inlining cost model.
Is the point here that you want to effectively double the bonus by both applying it as a bonus in the generic model and as a threshold adjustment in AMDGPU TTI?

Yes. IIUC, the generic model doesn't seem to have a target dependent approach to adjust the bonus. The default value is not sufficient and we want to inline it regardless.

I think it would be cleaner to make that bonus configurable via TTI. Splitting it across two places is pretty confusing...

[shiltian]

Sure. I can do that.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.