Add MC support for CHERI memory colouring instructions. #508
Conversation
Based on the draft addition to the ISA spec. This is MC-only for now, though the CodeGen parts of this are likely to only be intrinsics and so not very difficult to implement. This should be sufficient to write tests for the Sail model.
| (ins GPR:$rs1, GPCR:$rs2), | ||
| "cstoreversion", "$rs1, $rs2">; | ||
| let mayLoad = 1, mayStore = 1 in | ||
| def CAmoCDecVersion : Cheri_rr<0x3, "camocdecversion", GPR>; |
There was a problem hiding this comment.
Is it important to indicate that $rs1 is atomic?
| def CAmoCDecVersion : Cheri_rr<0x3, "camocdecversion", GPR>; | |
| def CAmoCDecVersion : Cheri_rr<0x3, "camocdecversion", GPR, GPR, GPCRMemAtomic>; |
There was a problem hiding this comment.
I believe that type exists only to allow using 0(ca0) in the assembler despite the instruction not having an immediate operand.
There was a problem hiding this comment.
I've not seen that before, but in other back ends you never specify anything explicitly on operands to indicate that they are the memory addresses for atomics.
There was a problem hiding this comment.
Yes it's solely a parser thing. RISC-V's atomic operands are the only memory-accessing instructions that don't take an immediate, and have (rs1) as the canonical form but also accept 0(rs1), primarily to make it easy for generated assembly. Since our explicit loads and stores end up looking the same in terms of operands I reused the existing class (and its corresponding capability version), and did not rename it to avoid merge conflicts just for something cosmetic. In this case you do want the suggested change, but only so its assembly syntax matches the other AMOs, i.e. that 0(cs1) is permitted in the assembly.
There was a problem hiding this comment.
This being the case I am tempted to (ab)use GPCRMemAtomic for the address operands to cloadversion and cstore version too so that they get brackets around the address. Thoughts?
There was a problem hiding this comment.
Yeah, that's what I would do, then it matches the explicit loads/stores. Maybe one day I'll rename it to something more general (and do so upstream too, even if it currently makes less sense, but I think the hypervisor load/store-as-guest instructions are also immediate-less and will end up wanting the same syntax).
There was a problem hiding this comment.
I think I would like this to be a separate (dependent) extension, otherwise people may come along, see the instructions and be surprised that they don't work. I don't mind them living in the XCheri file though, especially as that'll reduce churn if and when they become less experimental and get folded into XCheri itself.
| @@ -1554,3 +1554,21 @@ let Predicates = [HasCheri, IsRV64, IsCapMode] in { | |||
| defm : CheriLdPat<load, CLC_128>; | |||
| defm : CheriStPat<store, CSC_128, GPCR>; | |||
| } // Predicates = [HasCheri, IsRV64, IsCapMode] | |||
|
|
|||
|
|
|||
| /// CHERI memory colouring extensions | |||
There was a problem hiding this comment.
Instructions go in the first half of the file to keep MC and CodeGen cleanly separated*
(* pseudos are more complicated, but placed based on whether they're used for assembly or just CodeGen)
| @@ -0,0 +1,23 @@ | |||
| # RUN: llvm-mc %s -triple=riscv64 -mattr=+xcheri -show-encoding \ | |||
There was a problem hiding this comment.
These belong in rv64xcheri-valid.s as it stands
There was a problem hiding this comment.
(though a separate extension means rv64xname-valid.s for some other name)
Add a +xcherimemversion feature that enables them.
1. Swap the position of CStoreVersion operands so that it conforms with other store operations (which are a bit irregular) 2. CAmoCDecVersion now takes a capability for expected version argument.
|
I've updated this to reflect some changes to the memory versioning spec. |
… and camocdecversion. This slightly uses GPCRMemAtomic which exists only to support memory operands without an immediate and could be renamed.
…ated on top of it.
| let mayLoad = 1, mayStore = 1, hasSideEffects = 0 in | ||
| def CAmoCDecVersion : RVInstR<0x3, 0, OPC_CHERI, (outs GPR:$rd), | ||
| (ins GPCRMemAtomic:$rs1, GPCR:$rs2), | ||
| "camocdecversion", "$rd, $rs1, $rs2">; |
There was a problem hiding this comment.
| "camocdecversion", "$rd, $rs1, $rs2">; | |
| "camocdecversion", "$rd, $rs2, $rs1">; |
to match stores and atomics
This uses @jrtc27's proposed name for the atomic RMW operation. @rmn30, let me know if you need anything beyond this to assemble test cases for Sail. @jrtc27, I've not put this behind a feature flag but it's likely to remain an optional extension on top of xCHERI for quite a while. We can either define these instructions to always exist but fail (most of them have a mechanism for reporting failure), always enable them in CodeGen but have them trap on some targets, or hide them behind a separate feature flag. I have no strong opinions either way - they're not likely to be used in IR lowering except via explicit use of intrinsics, so there isn't much of a problem enabling them unconditionally in the compiler and require code that uses them to explicitly check first.