This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][MC] Support TFE modifiers in MUBUF loads and stores.
ClosedPublic

Authored by kosarev on Nov 10 2022, 5:33 AM.

Diff Detail

Event Timeline

kosarev created this revision.Nov 10 2022, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2022, 5:33 AM
kosarev requested review of this revision.Nov 10 2022, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2022, 5:33 AM

The most of the changes are for tests where the presence of the TFE operands doesn't make much sense and the code changes seem compact enough to not bother with making non-existing instructions possible and then slashing them with custom checks as we do for MIMGs. Also agrees with the existing practice of having the LDS and Offset/Index/Both variants as separate instructions.

For some reason SP3 additionally accepts TFE forms with vN VData operands where v[N:N+1] is expected, so for example buffer_load_dword v1, v0, s[4:7], s1 glc tfe and buffer_load_dword v[1:2], v0, s[4:7], s1 glc tfe are both accepted and produce same opcodes. It should be possible to support that with a separate change, if needed.

Apart from that, https://reviews.llvm.org/D19584 mentions that use of off operands agrees with SP3. I wasn't able to find any signs in SP3 behaviour confirming this.

dp added a comment.Nov 10 2022, 6:19 AM

AFAIK, tfe should be supported for loads and atomics only. I think that there is no sense to use tfe with stores.
SP3 is very forgiving and does not check many limitations.

dp added a comment.Nov 10 2022, 6:30 AM

For some reason SP3 additionally accepts TFE forms with vN VData operands where v[N:N+1] is expected, so for example buffer_load_dword v1, v0, s[4:7], s1 glc tfe and buffer_load_dword v[1:2], v0, s[4:7], s1 glc tfe are both accepted and produce same opcodes. It should be possible to support that with a separate change, if needed.

I think we do not have to support this SP3 feature.

Apart from that, https://reviews.llvm.org/D19584 mentions that use of off operands agrees with SP3. I wasn't able to find any signs in SP3 behaviour confirming this.

Unfortunately, SP3 is not consistent in accepting 'off' for unused operands. It does support off for FLAT operands, but not for MUBUF.

AFAIK, tfe should be supported for loads and atomics only. I think that there is no sense to use tfe with stores.

That was my guess as well. But isn't it right that we still want to be able to disassemble TFE stores, even if functionally useless?

dp added a comment.Nov 10 2022, 7:47 AM

That was my guess as well. But isn't it right that we still want to be able to disassemble TFE stores, even if functionally useless?

You are right, that would be useful. But I suggest correcting the parser to trigger an error if a store is used with tfe.

You are right, that would be useful. But I suggest correcting the parser to trigger an error if a store is used with tfe.

Do you mind if we do it with a separate follow-up patch? We already have some TFE stores in tests and whatever we do with them here might be masked by the amount of other test changes.

arsenm accepted this revision.Nov 10 2022, 9:11 AM

LGTM. Parser error handling should be a separate patch anyway

This revision is now accepted and ready to land.Nov 10 2022, 9:11 AM
foad added inline comments.Nov 10 2022, 10:18 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
7704

Remove IsLds now it is unused?

kosarev updated this revision to Diff 474712.Nov 11 2022, 3:16 AM

Removed unused function parameter and cleaned up related code (thanks Jay!).

dp added inline comments.Nov 11 2022, 3:24 AM
llvm/lib/Target/AMDGPU/BUFInstructions.td
2286–2290

Is there a reason to use both isTFE and noTFE? The expressions like !not(noTFE) are difficult to read.

kosarev added inline comments.Nov 11 2022, 3:37 AM
llvm/lib/Target/AMDGPU/BUFInstructions.td
2286–2290

noTFE means we don't want the TFE version, for which isTFE is true, so not the same thing. !not(noTFE) indeed catches the eye, but I have no better options and it looks rather minor. Would appreciate any suggestions, though.

dp added inline comments.Nov 11 2022, 8:35 AM
llvm/lib/Target/AMDGPU/BUFInstructions.td
2443

noTFE is always 0 and may be removed.

2821

It looks like noTFE is always 0 and may be removed.

dp added inline comments.Nov 11 2022, 8:44 AM
llvm/lib/Target/AMDGPU/BUFInstructions.td
2286–2290

Maybe hasTFE or enableTFE (with default=1) would be more readable?

kosarev updated this revision to Diff 474801.Nov 11 2022, 10:32 AM

Removed unused TableGen class parameters and reworked noTFEs into hasTFEs.

dp accepted this revision.Nov 11 2022, 11:53 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Nov 14 2022, 7:36 AM
This revision was automatically updated to reflect the committed changes.