This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add .insn support for compressed formats.
ClosedPublic

Authored by craig.topper on Mar 22 2023, 2:38 PM.

Details

Summary

We've supported .insn for non-compressed for a while. This finishes the compressed supported.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 22 2023, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 2:38 PM
craig.topper requested review of this revision.Mar 22 2023, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 2:38 PM
jrtc27 added inline comments.Mar 22 2023, 2:43 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1614

This is a slightly weird error to give in this case... then again so is the non-C equivalent by the looks of it

2561

Wrap?

llvm/test/MC/RISCV/insn_c.s
2

Test errors?

craig.topper added inline comments.Mar 22 2023, 2:52 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1614

Why is it weird?

jrtc27 added inline comments.Mar 22 2023, 2:53 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1614

C0 through C2 aren't numbers, this kind of implies only 0 through 2 are accepted, not also C0 through C2.

MaskRay added inline comments.Mar 22 2023, 2:54 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2561

Consider llvm::is_contained({...}, Format) to reduce the length and therefore improve readability.

llvm/test/MC/RISCV/insn_c.s
3

--check-prefix instead of (es) if there is just one prefix.

craig.topper added inline comments.Mar 22 2023, 3:35 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1614

Do you want me to say "opcode must be a valid opcode name or an immediate in the range [0, 2]"?

If you've figured out the format for these directives you're probably also looking at the opcode list.

jrtc27 added inline comments.Mar 22 2023, 3:39 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1614

That sounds fine to me, thanks

-Use StringSwitch to check Format.
-Update error message.
-Fix check-prefix

Working on error test cases

Add error tests

craig.topper retitled this revision from [RISCV] Add .insn support compressed formats. to [RISCV] Add .insn support for compressed formats..Mar 22 2023, 4:31 PM
MaskRay added inline comments.Mar 22 2023, 8:54 PM
llvm/test/MC/RISCV/insn_c-invalid.s
5

For new tests, use [[#@LINE]]. [[@LINE]] is deprecated lit syntax.

Use #@LINE instead of @LINE

asb accepted this revision.Mar 27 2023, 1:51 AM

LGTM - I just wonder about if we want to gate this on C/Zca as this patch does. Assuming someone reused the 16-bit encoding space, perhaps they want to using .insn to insert some of their instructions. OTOH, perhaps it's not so likely that the instruction formats would be as they want, at which point .insn isn't very useful.

This revision is now accepted and ready to land.Mar 27 2023, 1:51 AM
This revision was landed with ongoing or failed builds.Mar 27 2023, 10:40 AM
This revision was automatically updated to reflect the committed changes.