Also makes a step towards resolving
https://github.com/llvm/llvm-project/issues/38652
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt | ||
---|---|---|
11357 | SP3 actually decodes these to the form with null. |
Please include a link to https://github.com/llvm/llvm-project/issues/38652 in the commit message, since it seems you are addressing at least part of it.
GFX9 does support soffset+offset, yes, but the encoding is different and so it probably deserves a separate patch.
Done.
Unfortunately if you change the commit message, arc diff does not automatically update the summary. You have to do it manually by clicking on Edit Revision.
llvm/lib/Target/AMDGPU/SMInstructions.td | ||
---|---|---|
751–752 | Can you just soffset instead of soffset{7-0}, here and elsewhere? | |
911–917 | These expressions seem over-complicated. Is it really important to set these fields to ? in the "!has_offset && !has_soffset" case, which presumably will never happen? Can you simplify to: let Inst{52-32} = !if(ps.has_offset, offset{20-0}, 0); let Inst{63-57} = !if(ps.has_soffset, soffset{6-0}, !cast<int>(SGPR_NULL.HWEncoding)); |
To clarify, you are deliberately only doing this for SMEM loads, not for stores or probes or any other SMEM instruction types? Given that, the patch looks OK to me, but I would appreciate a second opinion maybe from @dp.
Yes, that's just for soffset+offset loads. Feels like that's enough for a single patch.
llvm/lib/Target/AMDGPU/SMInstructions.td | ||
---|---|---|
751–752 | Sure, done. | |
911–917 | There are actually SMEMs that do not employ the fields, e.g., s_dcache_inv and s_memrealtime. So yes, I think we need the ?s. Added a comment explaining that. |
The patch looks fine to me.
I'd have added a couple of tests for new addressing mode to validate offset limitations, e.g.
s_load_dwordx16 s[20:35], s[4:5], s0 offset:0x1FFFFF s_buffer_load_dwordx16 s[20:35], s[4:7], s0 offset:-1
should trigger an error.
Can you just soffset instead of soffset{7-0}, here and elsewhere?