[PATCH] D102231: [AMDGPU][AsmParser/Disassembler] Correct A16 and G16 handling

[David Stuttard via Phabricator via llvm-commits]

dstuttard added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:159
+unsigned getAddrSizeMIMGOp(const MIMGBaseOpcodeInfo *BaseOpcode,
+                           const MIMGDimInfo *Dim, bool IsA16, bool NoG16) {
+  unsigned AddrWords = BaseOpcode->NumExtraArgs;
----------------
dp wrote:
> I'd have renamed "NoG16" to "IsG16Supported" though this is a matter of taste.
Yes, I think you are right. I'll make that change.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:169
+  if (BaseOpcode->Gradients) {
+    if ((IsA16 && NoG16) || BaseOpcode->G16)
+      // There are two gradients per coordinate, we pack them separately.
----------------
dp wrote:
> Am I correct that A16 do affect gradient packing when G16 is not supported? In other words, A16 affects gradient packing on GFX9, right? (gfx9_shader_programming is silent about packing.)
> 
> As this is a badly documented feature, could you add a comment explaining how G16 and A16 affect gradient packing?
> 
> I compared the tests with your original patch and they are identical. Probably a few extra tests for _d/_cd opcodes with different combinations of A16 and G16 would be useful to have.
Yes, that's right. If G16 isn't supported, A16 implies G16. (Yes, for gfx9 a16 makes gradients 16 bit too).

Yes, I'll add a comment to that effect.

You're correct, there aren't any new tests. I'll enhance those too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102231