This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support hypervisor extention instructions
ClosedPublic

Authored by tangxingxin1008 on Jan 19 2022, 4:55 PM.

Details

Summary
According to privileged spec version-20211203

Add the following hypervisor instructions::
        - HLV.B HLV.BU
        - HLV.H HLV.HU HLVX.HU
        - HLV.W HLV.WU HLVX.WU
        - HLV.D
        - HSV.B HSV.H HSV.W HSV.D

Signed-off-by: eric.tang <eric.tang@starfivetech.com>

Diff Detail

Event Timeline

tangxingxin1008 requested review of this revision.Jan 19 2022, 4:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 4:55 PM
jrtc27 added inline comments.Jan 19 2022, 5:11 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
462

This doesn't make sense to me. Why do we have an operand that's always zero and used to fill in Inst bits? Surely these should be hard-coded to 0 as part of the encoding and GPRMemAtomic (which should probably be renamed to something more general... we already abuse it downstream for non-atomic instructions) used for $rs1 to get the immdiate-less memory register operand parsing?

Though in this case shouldn't it just be a funct5? And using an RVInstR with let rs2 = funct5 (see the atomics for example)?

478

Similar to loads, should just be let rd = 0, surely, with GPRMemAtomic for rs1?

761

You get this all for free with GPRMemAtomic

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

Good points. Thank you for your suggestions, I will change it later.

478

Good point.

Address jrtc27's comment. Thanks.

tangxingxin1008 edited the summary of this revision. (Show Details)Jan 19 2022, 11:02 PM
asb added a comment.EditedJan 24 2022, 5:23 AM

Thanks for the patch. Alongside the inline comments, two minor suggestions:

  • There's no need to split it out now I don't think, but the GPRAtomicMemOp->GPRMemZeroOffset is a change that could have been split out to a separate commit and reviewed separately.
  • We don't do enough testing for this in general I don't think, but it would be good to add tests that attempting to use the RV64-only hypervisor instructions on RV32 produces an error.

Once the inline comments are addressed and tests added for using the RV64 instructions on RV32, I think it should be ready to land.

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

I know this comment is copied, but as you're touching it perhaps reword to "A parse method for (${gpr}) or 0(${gpr}), where the 0 will be silently ignored." I'd drop the "Used for GNU as compatibility" bit as I don't think it's adding much information.

743

For a block of instructions like this, we typically place the : at the some column on every line, meaning the right hand side is aligned (and so easy to scan to check the integer parameters).

751

The general preference is to match the order of instructions in the instruction listing table (table 9.1 in the ratified privileged spec update). This would put the RV64 only instructions in a group after the load+store instructions that are available in both RV32 and RV64.

llvm/test/MC/RISCV/priv-invalid.s
1

If anything, it would be better to add a second RUN line for riscv64 alongside the existing riscv32. You could then have priv-rv64-invalid.s for the RV64 only instructions.

@tangxingxin1008: Might you have a chance to look at these review comments soon? I've ended up rebasing D117432 on top of this patch in order to use GPRMemZeroOffset, so that's another reason it would be good to land this. Thanks.

asb added a comment.Feb 16 2022, 5:56 AM

@tangxingxin1008: Might you have a chance to look at these review comments soon? I've ended up rebasing D117432 on top of this patch in order to use GPRMemZeroOffset, so that's another reason it would be good to land this. Thanks.

Just a friendly ping on this.

@tangxingxin1008: Might you have a chance to look at these review comments soon? I've ended up rebasing D117432 on top of this patch in order to use GPRMemZeroOffset, so that's another reason it would be good to land this. Thanks.

Just a friendly ping on this.

I'm very sorry so late back to you. I will update it as soon as possible.

Thanks for the patch. Alongside the inline comments, two minor suggestions:

  • There's no need to split it out now I don't think, but the GPRAtomicMemOp->GPRMemZeroOffset is a change that could have been split out to a separate commit and reviewed separately.
  • We don't do enough testing for this in general I don't think, but it would be good to add tests that attempting to use the RV64-only hypervisor instructions on RV32 produces an error.

Once the inline comments are addressed and tests added for using the RV64 instructions on RV32, I think it should be ready to land.

" There's no need to split it out now I don't think", do you mean the two commits [D117654, D117733] should be to one commit?

asb added a comment.Feb 17 2022, 5:53 AM

Thanks for the patch. Alongside the inline comments, two minor suggestions:

  • There's no need to split it out now I don't think, but the GPRAtomicMemOp->GPRMemZeroOffset is a change that could have been split out to a separate commit and reviewed separately.
  • We don't do enough testing for this in general I don't think, but it would be good to add tests that attempting to use the RV64-only hypervisor instructions on RV32 produces an error.

Once the inline comments are addressed and tests added for using the RV64 instructions on RV32, I think it should be ready to land.

" There's no need to split it out now I don't think", do you mean the two commits [D117654, D117733] should be to one commit?

Sorry that was a big ambiguous. I just meant that although splitting out the GPRMemZeroOffset change would be helpful, but not essential. But you've done it in D120017 now anyway, which is helpful as it should unblock my other patch. Thanks!

Address asb's comment.

tangxingxin1008 retitled this revision from [RISCV] Update Privileged spec to version-20211203: Support Hypervisor Extention to [RISCV] Support hypervisor extention instructions.Feb 18 2022, 12:38 AM
tangxingxin1008 edited the summary of this revision. (Show Details)
tangxingxin1008 marked 4 inline comments as done.Feb 18 2022, 12:43 AM
asb added a comment.Feb 24 2022, 4:14 AM

Thanks, there's just one piece (see below) that was missed and otherwise this looks great to me.

Could you please add test coverage for the RV64-only instructions on RV32 (i.e. demonstrating attempting to use them produces an error).

tangxingxin1008 marked an inline comment as not done.

Address asb's comment. Add more tests for the RV64-only instructions on RV32.

asb accepted this revision.Feb 25 2022, 12:27 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Feb 25 2022, 12:27 AM
This revision was landed with ongoing or failed builds.Feb 27 2022, 10:08 PM
This revision was automatically updated to reflect the committed changes.