RISC-V: add rule about target features vs. misa extensions - #2354
TechnoPorg wants to merge 4 commits into
Conversation
rustbot
added
the
S-waiting-on-review
| > [!NOTE] | ||
| > Some RISC-V standard extensions can be enabled (1) or disabled (0) via their corresponding bits in the [Machine ISA][rv-machine] (`misa`) register. | ||
| > For example, the [A][rv-a] and [M][rv-m] bits being cleared means atomic instructions and integer multiplication/division instructions are unimplemented. | ||
| > | ||
| > Rust code compiled with a RISC-V target feature of the same name as an extension assumes the extension will always be available. | ||
| > It is undefined behaviour to execute it in an environment where the extension is disabled (bit is 0 in `misa`). |
There was a problem hiding this comment.
I think a note (which is not normative) is not enough here. We need to say, normatively, that code compiled with a "single-letter target feature" (or whatever they are called) is UB to invoke unless the corresponding bit is actually set in the misa register.
There was a problem hiding this comment.
Would that go in the same part of the document, just not as a note?
There was a problem hiding this comment.
I'd expect the rule, or a generalization of it, to go in Behavior Considered Undefined; then we'd add admonitions (notes) in other places where relevant (such as in codegen) pointing to that.
There was a problem hiding this comment.
It's already there.
The point here is that we should clarify what "the current platform does not support" means specifically for RISCV target features that are impacted by the misa register. I would not expect such target-specific definitions to be listed on the general UB page.
There was a problem hiding this comment.
Right. I was looking at the rule as written in the quote above, "it is undefined behavior to execute it in an environment where ...". Rules of that form are what I'd expect to go where mentioned.
As you say, probably what we need here instead is a normative statement about how what we're discussing affects what's supported on the platform. Then it (or perhaps an accompanying admonition) can cite undefined.target-feature rather than reiterating that substance.
As for where that rule should go, I'm open to ideas. I'd probably expect to see it in the codegen chapter, given the current organization of things.
There was a problem hiding this comment.
Thanks! I've reworded it and made it a rule.
TechnoPorg
changed the title
|
|
||
|
|
||
| r[attributes.codegen.target_feature.riscv.misa] | ||
| Some targets implement the [Machine ISA] (`misa`) register. Whether a single-letter extension (`A`, `B`, ...) is present can be queried from the corresponding bit of this register. |
There was a problem hiding this comment.
"Some targets"? As in, only a subset of riscv targets?
There was a problem hiding this comment.
If the target doesn't implement the privileged architecture (quite unlikely? I haven't found any target that actually falls into this category, although the spec reads as though it's allowed), Machine ISA won't be implemented at all.
If it does implement the privileged architecture, it's legal for misa to return zero (spec), meaning it's effectively unimplemented or there's some other target-specific way to query features.
There was a problem hiding this comment.
| Some targets implement the [Machine ISA] (`misa`) register. Whether a single-letter extension (`A`, `B`, ...) is present can be queried from the corresponding bit of this register. | |
| Some RISC-V targets implement the [Machine ISA] (`misa`) register. Whether a single-letter extension (`A`, `B`, ...) is present can be queried from the corresponding bit of this register. |
So maybe this is more clear then.
Co-authored-by: Ralf Jung <post@ralfj.de>
Ah sorry, didn't realize that, and thanks for providing feedback anyways. I'll be more careful going forward about who I ping. |
This branch has not been deployed
5 participants
As per the discussion in rust-lang/rust#162552 and #t-compiler/risc-v > RISC-V extensions vs. Rust target features, this clarifies that enabling a target feature means the
misabit of the same name (if any) is assumed to always be set. Although this originally came up in the context of theMextension, it applies to many others as well, which are listed in the linked Machine ISA manual page.cc @RalfJung