[PATCH] D68873: [AMDGPU] Amend target loop unroll defaults

[Tim Corringham via Phabricator via llvm-commits]

timcorringham added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:62
 
 static cl::opt<unsigned> UnrollThresholdLocal(
   "amdgpu-unroll-threshold-local",
----------------
nhaehnle wrote:
> rampitec wrote:
> > This change penalizes loops which should have unroll boosted instead. Your new default thresholds are now higher than boosted.
> I see now change here. Is something weird going on with the diff?
I now initialise ThresholdLocal to be the max of UnrollThresholdLocal and UP.Threshold., so the value used will only be increased for PAL.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:93
                                             TTI::UnrollingPreferences &UP) {
   UP.Threshold = 300; // Twice the default.
   UP.MaxCount = std::numeric_limits<unsigned>::max();
----------------
arsenm wrote:
> This would now be dead
This value is still used if the OS isn't PAL.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:100
+  // Set more aggressive defaults for PAL shaders
+  if (TargetTriple.getOS() == Triple::AMDPAL) {
+    UP.MaxPercentThresholdBoost = 1000;
----------------
arsenm wrote:
> These should probably be the same for all OSes
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. 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h:52
 
+  AMDGPUSubtarget::Generation Gen;
+
----------------
arsenm wrote:
> You don't need to add this field. You already have the subtarget available here, you just need to change the type
Good point. I didn't pay enough attention when I resolved the merge of my code - fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68873/new/

https://reviews.llvm.org/D68873