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

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.
This revision is now accepted and ready to land.

Looks reasonable, though I can't follow all the logic in packImage16bitOpsToDwords.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:1505
+  // A16 implies 16 bit gradients if subtarget doesn't support G16
+  if (IsA16 && !AMDGPU::hasG16(STI) && !IsG16)
     return false;
----------------
"STI.hasG16()"


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4034
+               (I >= Intr->CoordStart && !IsA16)) {
+      // Handle any non-gradient / non-coordinate operands, plus any gradient
+      // or coordinate operands that should not be packed
----------------
Aren't "non-gradient / non-coordinate operands" handled in the "if" above? Also I don't understand why the case above has a bitcast but this case does not. But I see that that was the pre-existing behaviour.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4233
+  if (BaseOpcode->Gradients && !ST.hasG16() && (IsA16 != IsG16))
+    // 16 bit gradients are supported, but are tied to the A16 control
+    // so both gradients and addresses must be 16 bit
----------------
Braces around this multi-line if body please (maybe `git clang-format @^` can do this for you?).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4238
+  if (IsA16 && !ST.hasA16())
+    // A16 not supported
+    return false;
----------------
Likewise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102066