This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support named opcodes in .insn directive.
ClosedPublic

Authored by Nelson1225 on Dec 7 2021, 1:11 AM.

Details

Summary

This patch is one of the TODO of commit, 283879793dc787225992496587581ec77b6b0610

We build the GenericTable for these opcodes, and also extend class RISCVOpcode, to store the names of opcodes. Then we call the parseInsnDirectiveOpcode to parse the opcode filed in .insn directive. We only allow users to write the recognized opcode names, or just write the immediate values in the 7 bits range.

Documentation: https://sourceware.org/binutils/docs-2.37/as/RISC_002dV_002dFormats.html

Diff Detail

Event Timeline

Nelson1225 created this revision.Dec 7 2021, 1:11 AM
Nelson1225 requested review of this revision.Dec 7 2021, 1:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 1:11 AM
craig.topper added inline comments.Dec 8 2021, 3:19 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
385 ↗(On Diff #392304)

I copied these changes to D115172 from your internal code review before you posted this. They've been committed now along with moving the line break to after the memonic string. Can you rebase this patch?

jrtc27 added inline comments.Dec 8 2021, 3:26 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
183

Needs a line between these

llvm/test/MC/RISCV/insn-invalid.s
28

Group these with the other invalid operand tests? i.e. leave the .insn_i test as a special case at the end of the file

Nelson1225 edited the summary of this revision. (Show Details)
  • For llvm/test/MC/RISCV/insn-invalid.s, moved the new invalid test case forward, to leave the .insn_i test as a special case at the end of the file.
  • For llvm/lib/Target/RISCV/RISCVInstrInfo.td, add a newline between "def InsnDirectiveOpcode" and "def uimm7_opcode".
Nelson1225 marked 3 inline comments as done.Dec 9 2021, 12:49 AM

Thanks for your review and suggestions, Craig and Jessica. All fixed.

Nelson

llvm/lib/Target/RISCV/RISCVInstrInfo.td
183

Thanks, fixed.

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
385 ↗(On Diff #392304)

Sure, thanks for the reminder.

llvm/test/MC/RISCV/insn-invalid.s
28

Agreed, move the new testcase forward, and leave the .insn_i test at the end looks better.

LGTM to me with that formatting request.

llvm/lib/Target/RISCV/RISCVInstrFormats.td
126

Can we add extra spaces after the commas so that all the bit patterns start at the same column. That will make it more readable I think.

Nelson1225 marked 3 inline comments as done.

For llvm/lib/Target/RISCV/RISCVInstrFormats.td:126, add extra spaces after the commas to make the name and values aligned, this should looks more readable.

This revision is now accepted and ready to land.Dec 13 2021, 8:57 PM
This revision was landed with ongoing or failed builds.Dec 13 2021, 9:15 PM
This revision was automatically updated to reflect the committed changes.