This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Incorrect error message regarding SCC modifier
AcceptedPublic

Authored by jwanggit86 on Aug 30 2023, 4:34 PM.

Details

Reviewers
arsenm
Group Reviewers
Restricted Project
Summary

For the AMD GFX90A GPU, the SCC instruction modifier is allowed
for certain classes of instructions. However, the current assembler
generates an error message, "scc is not supported on this GPU",
regardless of the instruciton. This fix modifies the message as well
as the logic for generating the message. Related tests are moved from
gfx90a_err.s to gfx90a_asm_features.s.

Diff Detail

Event Timeline

jwanggit86 created this revision.Aug 30 2023, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2023, 4:34 PM
jwanggit86 requested review of this revision.Aug 30 2023, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2023, 4:35 PM
arsenm added inline comments.Aug 30 2023, 4:38 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490

I thought we had predefined masks for all the memory instruction types

llvm/test/MC/AMDGPU/gfx90a_err.s
198

needs to add the positive tests for these successfully parsing

arsenm requested changes to this revision.Aug 30 2023, 4:39 PM
This revision now requires changes to proceed.Aug 30 2023, 4:39 PM
arsenm added a reviewer: Restricted Project.Aug 30 2023, 4:39 PM
jwanggit86 edited the summary of this revision. (Show Details)

(1) created a const to hold the flags for all the 4 classes of instructions (2) tests removed from gfx90a_err.s are added to gfx90a_asm_features.s.

arsenm added inline comments.Aug 31 2023, 3:10 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490
4480–4490

apply clang-format, line looks too long

Made 2 changes per review comments: (1) var name changed from scc_flags to HasSCCInsts (2) code formatted with clang-format.

arsenm added inline comments.Sep 5 2023, 1:17 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490

SCCInsts isn't accurate. HasScalarCacheControl?

jwanggit86 added inline comments.Sep 6 2023, 12:38 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490

I thought SCC stands for System Cache Coherence. Anyway, SCC is not spelled out in this function, or anywhere else. A more accurate name for this tmp var is probably "AllowSCC" b/c the rhs is the flags representing 4 classes of instructions that allow SCC. Your thoughts?

arsenm added inline comments.Sep 6 2023, 12:46 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490

SCC is also a completely unrelated register so the whole thing is confusing

jwanggit86 added inline comments.Sep 6 2023, 2:30 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490

How about "AllowSCCModifier"? That should make it clear that we are referring to the modifier, not the register.

arsenm added inline comments.Sep 7 2023, 9:54 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4480–4490

Sure

Changed local var name from "HasSCCInsts" to "AllowSCCModifier".

arsenm accepted this revision.Sep 7 2023, 11:02 AM
This revision is now accepted and ready to land.Sep 7 2023, 11:02 AM