[AMDGPU] Fix -amdgpu-inline-arg-alloca-cost

[rampitec]

Summary

Before D94153 this threshold was in a pre-scaled units.
After D94153 inlining threshold multiplier is not applied
to this portion of the threshold anymore. Restore the
threshold by applying the multiplier.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

1192 ↗

(On Diff #329708)

Should the just adjust for the scale then?

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

1192 ↗

(On Diff #329708)

I thought about this, but whenever we will adjust the scale the next time we will have to visit it again.

[dfukalov]

Do you have any test for the fix?

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

1192 ↗

(On Diff #329708)

Nit: It seems instead of this modification you can just swap two lines

1582:  Threshold *= TTI.getInliningThresholdMultiplier();
1583:  Threshold += TTI.adjustInliningThreshold(&Call);

in InlineCost.cpp so we'll stay with just one place of * getInliningThresholdMultiplier().

[rampitec]

In D98362#2619375, @dfukalov wrote:

Do you have any test for the fix?

These tests tend to be either unreliable or huge. We cannot measure performance with lit tests.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

1192 ↗

(On Diff #329708)

That would change behavior for all targets.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

1192 ↗

(On Diff #329708)

I thought the point of the multiplier was to just amplify the expense of calls. I don't understand scaling up the cost here

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

1192 ↗

(On Diff #329708)

It's more like a uniform target cost multiplier which shall be applied to everything. But probably Daniil is correct and I have to swap two lines in the inliner instead, until noone uses it as is.

[rampitec]

Swapped order of operations in the InlineCost itself instead. This is still unused by any other target so it is possible to fix it there, which seems to be a more correct way.

[rampitec]

Added test.

[dfukalov]

Thanks!

llvm/lib/Analysis/InlineCost.cpp

if (!Caller->hasOptSize() && HotCallSiteThreshold) {
         // reduction, it can cause the size of a non-cold caller to increase
         // preventing it from being inlined.
         DisallowAllBonuses();
         Threshold = MinIfValid(Threshold, Params.ColdThreshold);
       }
     }
   }
 
+  Threshold += TTI.adjustInliningThreshold(&Call);
+
   // Finally, take the target-specific inlining threshold multiplier into
   // account.
   Threshold *= TTI.getInliningThresholdMultiplier();
-  Threshold += TTI.adjustInliningThreshold(&Call);
 
   SingleBBBonus = Threshold * SingleBBBonusPercent / 100;
   VectorBonus = Threshold * VectorBonusPercent / 100;
 
   bool OnlyOneCallAndLocalLinkage =
       F.hasLocalLinkage() && F.hasOneUse() && &F == Call.getCalledFunction();
   // If there is only one call of the function, and it has internal linkage,
   // the cost of inlining it drops dramatically. It may seem odd to update

llvm/test/Transforms/Inline/AMDGPU/amdgpu-inline-alloca-argument-cost.ll

+; RUN: opt -mtriple=amdgcn--amdhsa -S -passes=inline -inline-threshold=0 -debug-only=inline-cost < %s 2>&1 | FileCheck %s
+
+; REQUIRES: asserts
+
+target datalayout = "A5"
+
+; Verify we are properly adding cost of the -amdgpu-inline-arg-alloca-cost to the threshold.
+
+; CHECK: NumAllocaArgs: 1
+; CHECK: Threshold: 66000
+
+define void @use_private_ptr_arg(float addrspace(5)* nocapture %p) {
+  ret void
+}
+
+define amdgpu_kernel void @test_inliner_pvt_ptr(float addrspace(1)* nocapture %a, i32 %n) {
+entry:
+  %pvt_arr = alloca [64 x float], align 4, addrspace(5)
+  %to.ptr = getelementptr inbounds [64 x float], [64 x float] addrspace(5)* %pvt_arr, i32 0, i32 0
+  call void @use_private_ptr_arg(float addrspace(5)* %to.ptr)
+  ret void
+}