Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MC support for CHERI memory colouring instructions. #508

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

let Predicates = [HasCheri, IsRV64] in {
def CGetVersion : Cheri_r<0x13, "cgetversion">;
def CSetVersion : Cheri_rr<0x2, "csetversion">;
let mayLoad = 1 in
{
rmn30 marked this conversation as resolved.
Show resolved Hide resolved
def CLoadVersion : Cheri_r<0x14, "cloadversion">;
def CLoadVersions : Cheri_r<0x15, "cloadversions">;
}
let mayStore = 1, mayLoad = 0, hasSideEffects = 0 in
def CStoreVersion : RVInstCheriTwoSrc<0x7e, 0x2, 0, OPC_CHERI, (outs),
(ins GPR:$rs1, GPCR:$rs2),
rmn30 marked this conversation as resolved.
Show resolved Hide resolved
"cstoreversion", "$rs1, $rs2">;
let mayLoad = 1, mayStore = 1 in
def CAmoCDecVersion : Cheri_rr<0x3, "camocdecversion", GPR>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it important to indicate that $rs1 is atomic?

Suggested change
def CAmoCDecVersion : Cheri_rr<0x3, "camocdecversion", GPR>;
def CAmoCDecVersion : Cheri_rr<0x3, "camocdecversion", GPR, GPR, GPCRMemAtomic>;

Copy link
Member

@arichardson arichardson Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that type exists only to allow using 0(ca0) in the assembler despite the instruction not having an immediate operand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
23 changes: 23 additions & 0 deletions llvm/test/MC/RISCV/cheri/rv64xcheri-version.s
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# RUN: llvm-mc %s -triple=riscv64 -mattr=+xcheri -show-encoding \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These belong in rv64xcheri-valid.s as it stands

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(though a separate extension means rv64xname-valid.s for some other name)

# RUN: | FileCheck %s

# CHECK: cgetversion gp, csp
# CHECK: encoding: [0xdb,0x01,0x31,0xff]
CGetVersion x3, c2
rmn30 marked this conversation as resolved.
Show resolved Hide resolved
# CHECK: csetversion cra, csp, gp
# CHECK: encoding: [0xdb,0x00,0x31,0x04]
CSetVersion c1, c2, x3
# CHECK: cloadversion sp, cra
# CHECK: encoding: [0x5b,0x81,0x40,0xff]
CloadVersion x2, c1
# CHECK: cloadversions sp, cra
# CHECK: encoding: [0x5b,0x81,0x50,0xff]
CloadVersions x2, c1
# CHECK: cstoreversion ra, ctp
# CHECK: encoding: [0x5b,0x81,0x40,0xfc]
CStoreVersion x1, c4
# CHECK: camocdecversion ra, csp, gp
# CHECK: encoding: [0xdb,0x00,0x31,0x06]
CAmoCDecVersion x1, c2, x3