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

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
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>
----------------
dp wrote:
> kosarev wrote:
> > dp wrote:
> > > 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.
> > Yes, these are not needed for codegen needs and may only be useful for disassembling. Should we leave the TODOs be until we know for sure what to do with this?
> Let us see what other people here think. I'd have replaced TODOs with a description of limitations of the current design. But this is a matter of taste.
Tagging @foad @arsenm @rampitec for visibility.

Re replacing TODOs with descriptions: my only concern here would be to avoid masking what might be considered a real problem.


================
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]
----------------
dp wrote:
> kosarev wrote:
> > dp wrote:
> > > 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).
> > Agree, and so seem to do the other tests here. I don't mind to update them all, if that's the suggestion?
> The file has tests with s101, m0, etc for soffset so the coverage is sufficient. I suggest to correct one new test to use e.g. s1 instead of s0.
Done.

Note that there are still lots of other MC tests and test cases where only s0 is used at a register position. And then on a more general note, I admit I struggle a bit to see the point in testing all possible combinations of what encoding-, diagnostics- and implementation-wise seems to be completely unrelated, such as immediate/register operands and glc modifiers, if that's the right example. Feels like removing unnecessary repetitions would make uncovered cases more visible, allow more combinations that we are truly interested in and maybe somewhat reduce testing times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125700