[PATCH] D102066: [AMDGPU] Fix codegen of image intrinsics for g16 and a16

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Does globalisel get this right? Maybe add a -global-isel RUN line to the new test?



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6108-6109
+          dbgs()
+          << "Failed to lower image intrinsic: 16 bit addresses "
+             "need 16 bit addresses for 16 bit derivatives (got 32 bit)");
+      return Op;
----------------
"16 bit addresses need 16 bit addresses ..." reads wrong. But anyway maybe combine these two cases into a single "if (IsA16 != IsG16)" with an appropriately general error message?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6127
+  // gradient or both)
+  if (BaseOpcode->Gradients && IsG16 && !IsTiedG16) {
+    // Activate g16
----------------
Might be simpler to remove IsTiedG16 and just test `!ST->hasG16()` again here.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6138
+    // const int PackEndIdx = IsA16 ? VAddrEnd : (ArgOffset + Intr->CoordStart);
+    packImageA16AddressToDwords(
+        DAG, Op, GradPackVectorVT, VAddrs, ArgOffset + Intr->GradientStart,
----------------
Seems like there are some future cleanup opportunities here: packImageA16AddressToDwords shouldn't really have "A16" in its name since it's used for G16 too, and maybe A16/G16 should be passed in as an argument so the "if (A16)" test can be inside the helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102066