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

[David Stuttard via Phabricator via llvm-commits]

dstuttard added a comment.

See new patch with globalisel implementation



================
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;
----------------
foad wrote:
> "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?
I'm in two minds about this - I'd tend to err on the side of providing a more specific error message.
On balance, less code is better I guess.



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:6127
+  // gradient or both)
+  if (BaseOpcode->Gradients && IsG16 && !IsTiedG16) {
+    // Activate g16
----------------
foad wrote:
> Might be simpler to remove IsTiedG16 and just test `!ST->hasG16()` again here.
I was going to make a clarity argument - but I think you're right. It is just clutter.


================
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,
----------------
foad wrote:
> 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.
Yes, you're probably right. I've renamed the function for now (but not done the extra work to unify).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102066