This is an archive of the discontinued LLVM Phabricator instance.

Add RISCV privileged instructions
ClosedPublic

Authored by dvc94ch on Nov 23 2017, 12:38 AM.

Details

Summary

Adds the instructions defined in the RISCV privileged ISA. (uret, sret, mret, wfi, sfence.vma)

Diff Detail

Event Timeline

dvc94ch created this revision.Nov 23 2017, 12:38 AM
asb edited edge metadata.Nov 23 2017, 1:09 AM
asb added subscribers: llvm-commits, apazos, sabuasal and 2 others.

Hi David, many thanks for submitting this and welcome to the upstream LLVM community!

I'm adding the llvm-commits mailing list as a subscriber. Unfortunately this isn't done automatically and is necessary to ensure the patch is shared to that mailing list. There's a proposal to fix this annoyance.

I've left a few code review comments, all pretty minor stuff regarding naming and whitespace. Although the privileged spec only lists the I-type instruction form, it seems to me that {U,S,M}RET, WFI, and SFENCE.VMA are actually presented as R-type instructions.

Should uret, sret and mret have let isBarrier = 1, isReturn = 1, isTerminator = 1?

For anyone else reviewing this patch, these instructions are defined in the draft privileged spec: https://content.riscv.org/wp-content/uploads/2017/05/riscv-privileged-v1.10.pdf

lib/Target/RISCV/RISCVInstrInfo.td
583

The I use the _ri or _rr suffixes to differentiate reg-reg and reg-imm operands. As these instructions have no operands, just Prv or Priv would be the better class name.

611

SFENCE_VMA would match the naming convention of other instructions with a . in the name.

612

These two lines should be indented to aligned with the 0 in RVInstR<0...

test/MC/RISCV/priv.s
10 ↗(On Diff #124042)

Unwanted whitespace, also this patch doesn't add ecall.

14 ↗(On Diff #124042)

Unwanted whitespace, also this patch doesn't add ebreak.

asb added a comment.Nov 23 2017, 1:14 AM

One more thing - priv.s would be better named priv-valid.s, and it would be great to have a priv-invalid.s alongside it that checks that obviously invalid forms are rejected (e.g. sret with a register argument, sfence.vma with too few arguments).

dvc94ch updated this revision to Diff 124060.Nov 23 2017, 3:18 AM

Addressed all comments.

rename priv.s to priv-valid.s, added priv-invalid.s, removed ecall/ebreak test cases (where added because those instructions are explicitly mentioned in priv spec)

renamed tablegen class to Priv, added isTerminator, isBarrier, isReturn to mret, sret, uret

asb added a comment.Dec 5 2017, 5:02 AM

Is there any reason not to include the ecall/ebreak instruction definitions in this patch? (My apologies - I should have directly asked this when commenting on the fact that ecall/ebreak tests were present but not the definitions).

For what it's worth, the R-type format is now listed in the privileged ISA description https://github.com/riscv/riscv-isa-manual/commit/9f22e9a9d7b3e3d9649fb0a9d1d0b48821037222. Switching the definitions of URET, SRET, MRET, WFI, SFENCE.VMA to use RVInstR would be slightly preferable.

lib/Target/RISCV/RISCVInstrInfo.td
586

Along with some of the other backends, we typically treat let Foo = .. much like a C++ namespace declaration in terms of indentation (LLVM guidelines).

See RISCVInstrInfo.td for some examples.

dvc94ch updated this revision to Diff 126383.Dec 11 2017, 9:15 AM
  • Uses RVInstR as the base class for Priv
  • Fixed indentation
asb accepted this revision.Dec 12 2017, 7:15 AM

Thanks! This looks good to me.

This revision is now accepted and ready to land.Dec 12 2017, 7:15 AM
This revision was automatically updated to reflect the committed changes.