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

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp 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;
----------------
I'd have renamed "NoG16" to "IsG16Supported" though this is a matter of taste.


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102231