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

[Jay Foad via Phabricator via llvm-commits]

foad 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."
+>;
----------------
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.


================
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);
----------------
This part looks like a separate clean-up or latent bug fix?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4317
+      const bool UseNSA = ST.hasNSAEncoding() && PackedRegs.size() >= 3 &&
+                          PackedRegs.size() <= (unsigned)ST.getNSAMaxSize();
 
----------------
Don't need the cast


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4356
+    const bool UseNSA = ST.hasNSAEncoding() && CorrectedNumVAddrs >= 3 &&
+                        CorrectedNumVAddrs <= (unsigned)ST.getNSAMaxSize();
 
----------------
Don't need the cast (that was the whole point of making it unsigned).


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6189
+                VAddrs.size() >= 3 &&
+                VAddrs.size() <= (unsigned)ST->getNSAMaxSize();
   SDValue VAddr;
----------------
Don't need the cast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103348