[PATCH] D125700: [AMDGPU][GFX9] Support base+soffset+offset SMEM loads.

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp added a comment.

Overall looks good.



================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:544
+  // documentation suggesting otherwise.
+  // TODO: Ignore soffset_en when disassembling GFX8 instructions.
+  let Inst{14} = !if(ps.has_offset, ps.has_soffset, !if(ps.has_soffset, 0, ?));
----------------
kosarev wrote:
> Would love some opinions on how important we think it would be to support that. If I don't miss anything, addressing this would mean either further customising `AMDGPUDisassembler::getInstruction()` or split the TableGen definitions for GFX8 and GFX9 -- and not just the SMEM ones. Which seems to be a significant amount of changes.
>// TODO: Ignore soffset_en when disassembling GFX8 instructions.
Are there cases when an illegal GFX8 code may be decoded with soffset_en=1? The _SGPR_IMM_vi case should not work for GFX8, should it?


================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:559
+  // (no-offset + soffset) case, namely:
+  //   imm=0  soffset_en=1  offset=?  soffset=<soffset>
+  //   imm=1  soffset_en=1  offset=0  soffset=<soffset>
----------------
This addressing mode is semantically equivalent to the one where soffset is encoded in the offset field. I'm not sure we really need it. Note that sp3 has no syntactic sugar to enforce this encoding. It may be useful for decoder, but I doubt it worth the trouble.


================
Comment at: llvm/lib/Target/AMDGPU/SMInstructions.td:560
+  //   imm=0  soffset_en=1  offset=?  soffset=<soffset>
+  //   imm=1  soffset_en=1  offset=0  soffset=<soffset>
+
----------------
Ditto.


================
Comment at: llvm/test/MC/AMDGPU/gfx9_asm_smem.s:63
 
+s_load_dword s5, s[2:3], s0 offset:0x12345
+// CHECK: [0x41,0x41,0x02,0xc0,0x45,0x23,0x01,0x00]
----------------
It is a minor issue, but I noted that all your new tests use soffset=s0. It may be a good idea to avoid operands which are encoded as 0 (or add tests with other operands as well).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125700