[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: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, ?));
----------------
dp wrote:
> kosarev wrote:
> > dp wrote:
> > > kosarev wrote:
> > > > dp wrote:
> > > > > kosarev wrote:
> > > > > > dp wrote:
> > > > > > > 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?
> > > > > > Not sure I read the question right, but I understand, e.g., `0xc0024141 0x00012345` (with imm=1 and soffset_en=1) should decode to `s_load_dword s5, s[2:3], s0 offset:0x12345` in GFX9 and to `s_load_dword s5, s[2:3], 0x12345` in GFX8. With this patch in place we fail to do the latter because our GFX8 and GFX9 definitions share the same decoding namespace; the `isGFX9Only` predicate for the `_SGPR_IMM` case below makes it non-instruction in GFX8.
> > > > > So after this change disassembler will be unable to decode some legal GFX8 code, correct? I think this should be avoided. Would it be difficult to amend this patch with disassembler changes to avoid this breakage?
> > > > As there is no `soffset_en` in GFX8, all codes with that bit raised are not what I guess you call legal GFX8 codes. I think we would never normally produce such codes for GFX8 from codegen or assembly, but as of the moment I'm not aware of any reasons to think that such codes are actually illegal or invalid. That is basically a disassembling issue again.
> > > You are right, this is a corner case which does not look that important. However there are third-party tools which may produce such codes, you never know. We should be able to disassemble such codes unless this requires a lot of additional work.
> > Do we want to see it done as part of this patch? AFAIS, all the other notes are addressed.
> Probably a disassembler patch may be committed separately though I'd have preferred it as a part of this change.
>From what I see in how the TableGen's decoder backend works, it's fine to have predicated patterns that are special cases of other more generic patterns, even if the latter are themselves differently predicated. So as long as we keep our `isGFX9Only` instructions to be special cases of `isGFX8GFX9` ones, which I think we can expect being possible, disassembling should work for GFX8 as expected. For example, replacing the soffset_en expression with `let Inst{14} = !if(!and(ps.has_offset, ps.has_soffset), 1, ?);` resolves the decoding issue for the GFX8 instruction mentioned above.

Will update to support the GFX9 encodings for the _SGPR loads and add tests.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125700