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

Mon Jun 21 05:23:53 PDT 2021

foad accepted this revision.
foad added inline comments.
This revision is now accepted and ready to land.

================
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."
+>;
----------------
critson wrote:
> 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.
Fair enough

================
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);
----------------
critson wrote:
> 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.
Fair enough

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103348