[AMDGPU] Amend target loop unroll defaults

[timcorringham]

Summary

Amend the loop unroll thresholds for PAL shaders to be more aggressive.
This gives an overall performance benefit on a representative sample
of shaders.

[arsenm]

Could use a test

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

93–94

This would now be dead

101

These should probably be the same for all OSes

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h

52 ↗

(On Diff #224612)

You don't need to add this field. You already have the subtarget available here, you just need to change the type

[rampitec]

How big was the performance testing?

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

62

This change penalizes loops which should have unroll boosted instead. Your new default thresholds are now higher than boosted.

[nhaehnle]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

62

I see now change here. Is something weird going on with the diff?

[timcorringham]

Changes to address review comments

[timcorringham]

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

62

I now initialise ThresholdLocal to be the max of UnrollThresholdLocal and UP.Threshold., so the value used will only be increased for PAL.

93–94

This value is still used if the OS isn't PAL.

101

That is quite possible, but I don't have tests to confirm that for all OSes. Since the effect of crossing a cliff-edge can be significant (good or bad) I don't want to risk making that change for OSes without performance figures to justify it.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h

52 ↗

(On Diff #224612)

Good point. I didn't pay enough attention when I resolved the merge of my code - fixed.

[rampitec]

I disagree to the idea of having different thresholds based on the runtime. A runtime has nothing to do with it. For example compute can work on top of ROCm or PAL. Can you justify different results for the same programs?

I understand that you have some code or codes which benefit from a specific threshold. I suggest you to analyze these codes, understand and explain the performance gain root cause, then create a new heuristic in this function. That is why this function exists in the first place. It will also allow you to create a testcase.

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

62

It still does not make sense. You are initializing general threshold higher (1100) than boosted (1000).

[rampitec]

In addition it can be unrelated to the threshold at all. It may be a flaw in the cost model for specific instructions. Please also see D68881 which started to address cost model issues.

[timcorringham]

Function attribute for loop unroll threshold default

Changed approach for this in response to review comments. The new change is less invasive
and avoids OS/environment specific behavior in the target specific heuristics.
It allows the front-end to provide a minimum value for the loop unroll threshold on a
per-function basis, while still allowing the heuristics to adjust that threshold.

[rampitec]

This looks acceptable, but needs test.

[timcorringham]

Added test to confirm that the amdgpu-unroll-threshold attribute has the expected effect.

[rampitec]

LGTM

llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp

 
 #define DEBUG_TYPE "AMDGPUtti"
 
 static cl::opt<unsigned> UnrollThresholdPrivate(
   "amdgpu-unroll-threshold-private",
   cl::desc("Unroll threshold for AMDGPU if private memory used in a loop"),
   cl::init(2000), cl::Hidden);
 
 static cl::opt<unsigned> UnrollThresholdLocal(
   "amdgpu-unroll-threshold-local",
   cl::desc("Unroll threshold for AMDGPU if local memory used in a loop"),
   cl::init(1000), cl::Hidden);
 
 static cl::opt<unsigned> UnrollThresholdIf(
   "amdgpu-unroll-threshold-if",
   cl::desc("Unroll threshold increment for AMDGPU for each if statement inside loop"),
   cl::init(150), cl::Hidden);
for (const Value *V : I->operand_values()) {
     } else if (Depth < 10 && dependsOnLocalPhi(L, V, Depth+1))
       return true;
   }
   return false;
 }
 
 void AMDGPUTTIImpl::getUnrollingPreferences(Loop *L, ScalarEvolution &SE,
                                             TTI::UnrollingPreferences &UP) {
-  UP.Threshold = 300; // Twice the default.
+  const Function &F = *L->getHeader()->getParent();
+  UP.Threshold = AMDGPU::getIntegerAttribute(F, "amdgpu-unroll-threshold", 300);
   UP.MaxCount = std::numeric_limits<unsigned>::max();
   UP.Partial = true;
 
   // TODO: Do we want runtime unrolling?
 
   // Maximum alloca size than can fit registers. Reserve 16 registers.
   const unsigned MaxAlloca = (256 - 16) * 4;
   unsigned ThresholdPrivate = UnrollThresholdPrivate;
   unsigned ThresholdLocal = UnrollThresholdLocal;
   unsigned MaxBoost = std::max(ThresholdPrivate, ThresholdLocal);
   for (const BasicBlock *BB : L->getBlocks()) {
     const DataLayout &DL = BB->getModule()->getDataLayout();
     unsigned LocalGEPsSeen = 0;
 
     if (llvm::any_of(L->getSubLoops(), [BB](const Loop* SubLoop) {