This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GFX10] Support base+soffset+offset SMEM loads.
ClosedPublic

Authored by kosarev on May 6 2022, 11:58 AM.

Diff Detail

Unit TestsFailed

Event Timeline

kosarev created this revision.May 6 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 11:58 AM
kosarev requested review of this revision.May 6 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 11:58 AM
kosarev added inline comments.May 6 2022, 12:04 PM
llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt
11357

SP3 actually decodes these to the form with null.

foad added a comment.May 6 2022, 12:53 PM

I thought this was a GFX9 feature?

I thought this was a GFX9 feature?

Yes, sp3 allows it on gfx9 (except null of course).

foad added a comment.May 7 2022, 3:42 AM

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.

foad added a reviewer: dp.May 9 2022, 1:19 AM
kosarev updated this revision to Diff 428022.May 9 2022, 3:18 AM

Updated commit message as suggested.

I thought this was a GFX9 feature?

GFX9 does support soffset+offset, yes, but the encoding is different and so it probably deserves a separate patch.

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.

Done.

foad added a comment.May 9 2022, 3:23 AM

Updated commit message as suggested.

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.

kosarev edited the summary of this revision. (Show Details)May 9 2022, 3:38 AM
foad added inline comments.May 9 2022, 5:50 AM
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));
foad added a comment.May 9 2022, 5:54 AM

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.

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.

dp added a comment.May 9 2022, 1:24 PM

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.

kosarev updated this revision to Diff 428332.May 10 2022, 3:51 AM

Added validation tests.

foad accepted this revision.May 10 2022, 4:27 AM
This revision is now accepted and ready to land.May 10 2022, 4:27 AM
dp accepted this revision.May 10 2022, 6:08 AM

LGTM

This revision was landed with ongoing or failed builds.May 10 2022, 8:17 AM
This revision was automatically updated to reflect the committed changes.