Skip to content

Conversation

@arichardson
Copy link
Owner

Please take a look and let me know what you think before I send this upstream.

@jrtc27 @resistor @veselypeta @davidchisnall

@resistor
Copy link

resistor commented Oct 1, 2025

I'd like to pre-merge this with Cheriot before sending it upstream, if possible. Otherwise we won't be able to track upstream while I sort out all of the merge & test issues. I can start on that next week, most likely.

let isConstant = true in def X0_Y : RISCVCapReg<X0, "x0", ["zero", "null"]>,
DwarfRegAlias<X0>;
let CostPerUse = [0, 1] in {
def X1_Y : RISCVCapReg<X1, "x1", ["ra"]>, DwarfRegAlias<X1>;
Copy link

Choose a reason for hiding this comment

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

How would folks feel about supporting the "c"-prefixed names as a non-standard extension for backwards compatibility?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I definitely like this, but wasn't sure upstream would want it. Similarly adding aliases for the mnemonics downstream would make a lot of sense

Copy link

@resistor resistor Oct 1, 2025

Choose a reason for hiding this comment

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

I think upstream will be sympathetic to backwards compatibility so long as it does not compromise in the implementation too much. In this case it seems like it would be minimal to support?

Copy link
Owner Author

Choose a reason for hiding this comment

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

If adding it to altnames is sufficient then I imagine that could be fine

Copy link
Owner Author

Choose a reason for hiding this comment

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

I originally included support for the original mnemonics but that adds a lot of untested code upstream so probably better to keep that downstream?

Copy link

Choose a reason for hiding this comment

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

If adding it to altnames is sufficient then I imagine that could be fine

That's what I had in mind

Copy link

Choose a reason for hiding this comment

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

I originally included support for the original mnemonics but that adds a lot of untested code upstream so probably better to keep that downstream?

I don't think it matters to us too much. Our bincompat is for xcheri, which we will just have to have live in parallel with the new opcodes & mnemonics for now.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah the only shared part will be the register info change. I imagine allowing x names may start triggering weird diagnostics until we ignore the "wrong-mode" load/store instructions which currently never match due to the wrong register name. Morello has a IgnoredFeatures tablegen change to ignore them in the matcher and I should probably pull that in once I get to the loads and stores. So it might make sense for you to hold off integrating this into cheriot until I have that.

// fully backwards compatible with non-Y code).
def FeatureCapMode : SubtargetFeature<"cap-mode", "IsCapMode", "true",
"Capability pointer mode">;
def IsCapMode
Copy link

Choose a reason for hiding this comment

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

We renamed this to XCheriPureCap using RISCVExtension, so that it plays nice with the rest of the RISCV extension infrastructure. That enables us to do things like have XCheriot automatically imply XCheriPureCap.

Copy link

Choose a reason for hiding this comment

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

Though given the direction of the Y standard, perhaps we should invert the sense of the feature bit?

Copy link

Choose a reason for hiding this comment

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

Thoughts on this part?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm can't you have XCheriot imply the CapMode subtargetfeature?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It likely shouldn't have a x prefix since that is for vendor-extensions and the standard will have this.

Copy link

Choose a reason for hiding this comment

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

Hmm can't you have XCheriot imply the CapMode subtargetfeature?

We could do that in code, but the generic infrastructure for RV features implying other RV features only works for things declared as standard extensions or vendor extensions, and it then enforces the naming scheme.

Copy link

@resistor resistor Oct 9, 2025

Choose a reason for hiding this comment

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

Trying to page it all back in now, I think the issue might have been that CapMode can't imply XCheri (or Y in the future) because extensions can imply features, but features can't imply extensions.

Copy link
Owner Author

@arichardson arichardson Oct 9, 2025

Choose a reason for hiding this comment

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

Ah I was not aware that upstream introduced this restriction. I'm pretty sure you used to be able to imply certain non-extension features like CapMode.

arichardson and others added 2 commits October 5, 2025 22:48
This is the first commit in a series of changes to add initial MC-layer
support for the upcoming Y extension for CHERI.

Specification: https://riscv.github.io/riscv-cheri/
Co-authored-by: Jessica Clarke <[email protected]>
This adds initial features for the base RVY extension,
other extensions such as the hybrid mode will be added later.

Co-authored-by: Jessica Clarke <[email protected]>
Co-authored-by: Alexander Richardson <[email protected]>
Co-authored-by: Petr Vesely <[email protected]>
arichardson and others added 4 commits October 7, 2025 00:18
This adds MC-level support for most of the base Y extension instructions,
restricted to the execution-mode-independent subset. The Y extension
(CHERI for RISC-V) also introduces an execution mode that determines
whether certain register operands use the full extended register or only
the address subset (the current XLEN registers). The instructions that
depend on execution mode (loads/stores/jumps + AUIPC) will be added in
the next commit in this stack of changes.

Co-authored-by: Jessica Clarke <[email protected]>
Co-authored-by: Alexander Richardson <[email protected]>
Co-authored-by: Petr Vesely <[email protected]>
This helps avoiding diagnostics for instructions that could never be
selected and is required for RISC-V CHERI support.
An example here are the CHERI mode-dependent instructions where we have
loads/stores that are identical other than the register class for the
base register and have predicates that can never both be set. To avoid
nonsensical error messages, we should only use the candidate
instructions with the currently available feature bits.

For RVY (CHERI), loads and stores are mode-dependent, using either a
YLEN register or a XLEN register as the base. Prior to the
standardization process CHERI assembly used c-prefixed register names
for capabilities, so we had the following syntax for RISC-V compatible
mode and CHERI pure-capability mode:

lw x4, 0(c3)  # capability mode: use new `CLW` tablegen instruction
lw x4, 0(x3)  # integer mode: use existing `LW` tablegen instruction

During the standardization this was changed to keep the same register
name in both modes, so now we have `lw x4, 0(x3)` in both modes but we
have to select between two instructions: one using the normal GPR
register class and one using the YGPR register class.
We now have a choice between two instructions `LW` and `LW_Y` that have
predicates that can never both be true, so we should avoid reporting
missing predicates or wrong operands for the "unreachable" instruction.

This change was taken from Morello LLVM with a few minor comment
clarifications and changes to naming of variables.

Co-authored-by: Silviu Baranga <[email protected]>
This adds supports for all new RVY loads/stores (capability-wide
versions: ly/sy instructions). Additionally, for RVY (CHERI), loads and
stores are mode-dependent, using either a YLEN register or a XLEN
register as the base.
In the former case loads/stores are authorized by that register,
and in the latter (compatibility cast), the loads/stores keep using an
address but are authorized by the DDC CSR. The assembler mnemonics are
the same in both cases. Prior to the standardization process CHERI
assembly used c-prefixed register names for capabilities, so we had the
following syntax:

lw x4, 0(c3) # capability mode: use new `CLW` instruction
lw x4, 0(x3) # integer mode: use existing `LW` instruction

During the standardization this was changed to keep the same register
name in both modes, so now we have `lw x4, 0(x3)` in both modes but we
have to select between two instructions: one using the normal GPR
register class and one using the YGPR register class.
The newly added test checks that we select the right instruction
(`LW` or `LW_Y`) using --show-inst, since both the encoding and the
assembler syntax are the same in both modes.

This commit changes the Load_ri and Store_rri tablegen classes into a
multiclass that defines the RVI and RVY at the same time to reduce the
size of the diff and hopefully improve maintainability. The downstream
fork had duplicated definitions which avoids merge conflicts but does
mean any refactorings do not make it to the almost identical duplicate
definitions. The other advantage is that we also get support for the
other load/store instructions that are not explicitly tested in this
commit.
This ensures the broken Asmparser expansions trigger a crash
@jrtc27
Copy link

jrtc27 commented Oct 9, 2025

Given we have a new base ISA, I don't see why we need both I/E/Y and int/cap mode? We only have int/cap mode today because both are RVI, but you can just distinguish Y and not-Y, surely?

@arichardson
Copy link
Owner Author

Given we have a new base ISA, I don't see why we need both I/E/Y and int/cap mode? We only have int/cap mode today because both are RVI, but you can just distinguish Y and not-Y, surely?

We still need a feature bit to select I vs Y as the base ISA, this does not exist right now and is covered by capmode. Or are you suggesting we introduce something like BaseIsaI BaseIsaY features instead of the capmode ones?

@arichardson
Copy link
Owner Author

Right now plain loads have no predicates so we would need to add one?

@resistor
Copy link

Also I need to be able to rebase XCheriot on it, which is hindered if cap mode is not distinct from YBase

@jrtc27
Copy link

jrtc27 commented Oct 10, 2025

Given we have a new base ISA, I don't see why we need both I/E/Y and int/cap mode? We only have int/cap mode today because both are RVI, but you can just distinguish Y and not-Y, surely?

We still need a feature bit to select I vs Y as the base ISA, this does not exist right now and is covered by capmode. Or are you suggesting we introduce something like BaseIsaI BaseIsaY features instead of the capmode ones?

i and e are already feature strings (for FeatureStdExtI and FeatureStdExtE). With y (FeatureStdExtY) you can then use those for predicates.

@jrtc27
Copy link

jrtc27 commented Oct 10, 2025

Also I need to be able to rebase XCheriot on it, which is hindered if cap mode is not distinct from YBase

Then you just add your base ISA to the list of ones for the capability mode predicate, like I and E are both part of the integer mode predicate.

@jrtc27
Copy link

jrtc27 commented Oct 10, 2025

Right now plain loads have no predicates so we would need to add one?

Yes, but that's true regardless of whether the predicate is cap-mode or i-or-e.

@resistor
Copy link

Also I need to be able to rebase XCheriot on it, which is hindered if cap mode is not distinct from YBase

Then you just add your base ISA to the list of ones for the capability mode predicate, like I and E are both part of the integer mode predicate.

Ah, I didn't realize you were suggesting not having a feature but still retaining a tblgen predicate. I think that would work.

@arichardson
Copy link
Owner Author

Right now plain loads have no predicates so we would need to add one?

Yes, but that's true regardless of whether the predicate is cap-mode or i-or-e.

Yeah I think I misread you statement and we are all in agreement. We just need to figure out what the user-visible API to select the features should be.

Given we have the following four classes of instructions:

Instruction Available Base=I/E Available Base=Y Ext Current Extra Predicate
MUL ✔️ ✔️ StdExtI -
LW ✔️ StdExtI NotCapMode
LW_Y ✔️ StdExtY IsCapMode
YBASER ✔️ ✔️ StdExtY -

we will need the extra predicates for the mode-dependent instructions.
Do you suggest we use something like BaseIsaY instead of the existing capmode name?

XCheriot should be able to do something like

def FeatureVendorXCheriot
    : RISCVExtension<1, 0, "CHERIoT extension", [FeatureStdExtE, FeatureStdExtY, BaseIsaY]>;

@resistor
Copy link

I think the proposal was to treat XCheriot as its own base ISA, and define IsCapMode as explicitly checking for IsStdExtY || IsXCheriot

@arichardson
Copy link
Owner Author

arichardson commented Oct 10, 2025

I think the proposal was to treat XCheriot as its own base ISA, and define IsCapMode as explicitly checking for IsStdExtY || IsXCheriot

Oh I misread the previous ones, you are suggesting having both FeatureStdExtY and IsStdExtY? We still need to handle instructions like ybaser that work for both Y and I mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants