[PATCH] D134780: [AMDGPU] Add MIMG NSA threshold configuration attribute

Wed Sep 28 20:07:01 PDT 2022

critson added a comment.

In D134780#3821260 <https://reviews.llvm.org/D134780#3821260>, @foad wrote:

>>> Out of ~20000 pipelines, ~2000 had higher VGPR usage with threshold 2 and ~1000 had lower VGPR usage.
>>
>> That's weird. I can't see why enabling NSA would consistently cause higher vgpr usage.
>
> FYI I looked at one case where this happens and it was caused by GCNNSAReassign making strange (well, different) decisions. So now I need to try to understand what that pass does.

Yes, I think we probably need to revisit the logic in GCNNSAReassign.
I seem to remember it lack some support for subregisters which might be part of the issue.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:59
+                                      cl::desc("Number of addresses from which to enable MIMG NSA."),
+                                      cl::init(3), cl::Hidden);
+
----------------
foad wrote:
> On the "don't repeat yourself" principle you could either remove the cl::init here...
True, it would be nice to have this number only once.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.cpp:959
+  if (NSAThreshold.getNumOccurrences() > 0)
+    return std::max(NSAThreshold.getValue(), 2u);
+
----------------
foad wrote:
> I don't think you need the .getValue().
Pretty sure it is required, because std:max does not know how to handle cl::opt.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134780