[PATCH] D103348: [AMDGPU] Add maximum NSA size limit ISA feature

[Carl Ritson via Phabricator via llvm-commits]

critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPU.td:632-637
+class SubtargetFeatureNSAMaxSize <int Value> : SubtargetFeature <
+  "nsa-max-size-"#Value,
+  "NSAMaxSize",
+  !cast<string>(Value),
+  "The maximum non-sequential address size in VGPRs."
+>;
----------------
foad wrote:
> It seems a bit odd to define it like this gives the impression that the limit varies on different subtargets by design, which it doesn't really. The only reason to limit it to 5 is to "avoid stability issues" (i.e. work around hardware bugs). But I'm not sure what else to suggest.
I agree it is a bit odd, but does provide us flexibility if we need to change the limit again for other hardware issues.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4097-4101
+    if ((I < Intr->GradientStart) ||
+        (I >= Intr->GradientStart && I < Intr->CoordStart && !IsG16) ||
+        (I >= Intr->CoordStart && !IsA16)) {
       // Handle any gradient or coordinate operands that should not be packed
+      AddrReg = B.buildBitcast(V2S16, AddrReg).getReg(0);
----------------
foad wrote:
> This part looks like a separate clean-up or latent bug fix?
It is a fix for a bug triggered by changes in this diff.
I can move it to another review, but I do not think I'll be able to build a test case for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103348