This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add default 1 glc operand to rtn atomics
ClosedPublic

Authored by rampitec on Nov 3 2020, 4:25 PM.

Details

Summary

This change adds a real glc operand to the return atomic
instead of just string " glc" in the middle of the asm
string.

Improves asm parser diagnostics.

Diff Detail

Event Timeline

rampitec created this revision.Nov 3 2020, 4:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2020, 4:25 PM
rampitec requested review of this revision.Nov 3 2020, 4:25 PM
rampitec updated this revision to Diff 302974.Nov 4 2020, 2:20 PM
rampitec retitled this revision from [AMDGPU] Add default 1 glc operand to flat rtn atomics to [AMDGPU] Add default 1 glc operand to rtn atomics.
rampitec edited the summary of this revision. (Show Details)

Added the same change to MUBUF.

dp accepted this revision.Nov 5 2020, 4:07 AM

LGTM with a minor issue

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
401

Could MTBUF be removed?

This revision is now accepted and ready to land.Nov 5 2020, 4:07 AM
scott.linder added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
337–338

Is the _1 variant added here rather than replacing the existing one to support an old syntax, or to support disjoint classes of instructions which need different defaults/validation, or something else? I'm not sure I understand, but renaming the _1 to something more descriptive or adding a comment might help. It also seems like the other isX functions are all checking for something distinct (i.e. there is ImmTyOffset, ImmTyOffset0, and ImmTyOffset1 above), is that not needed here?

rampitec updated this revision to Diff 303159.Nov 5 2020, 10:13 AM
rampitec marked an inline comment as done.

Addressed review comments.

rampitec marked an inline comment as done.Nov 5 2020, 10:13 AM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
337–338

This is MatchClass of the GLC_1 operand, and GLC_1 operand is added the same way as GLC_0 was added, this is the default (and in this case forced) value of the GLC operand. It literally matches GLC_1 operand in the instruction inputs. Added comment.

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
401

Yep, at the end MTBUF does not need it as it does not have atomics.

This revision was automatically updated to reflect the committed changes.
rampitec marked an inline comment as done.