r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

The changes look fine (you could consider a macro, but it's not that important).

In many other cases a #[target_feature(enable = "...")] would work, but for the options here it looks like we don't (currently?) have a feature. So, why is it OK to emit these instructions?

It works because .option handling and instruction availability checking in general are handled by the assembler, not by the Rust compiler itself.
So as long as the assembler supports given extension (like h which is missing in Rust), it's okay (I checked that LLVM 20 ― the current minimum version ― supports all of them).

What I'm asking is why not guarding these intrinsics with a target feature is OK. In most other cases, intrinsics are annotated with some target feature, and then the caller must somehow guarantee that the target feature is actually available in practice. If the caller lies and the target feature is not actually available, you might run into an invalid instruction (SIGILL or similar).

So, why is there no target feature required here? is the h extension just always implicitly enabled? Also why is h missing in rust?

Ah, I completely misunderstood your question.

For the pause intrinsic (requiring zihintpause), missing #[target_feature] attribute is completely intended. Although that using this assembler instruction requires the Zihintpause extension to be present, this is only a hint and should be available even if this extension is not available on the target machine (in this case, pause would effectively work as a no-op).

For others... honestly, I don't know (I just made minimum changes to improve readability).
Neither h nor svinval exists in the Rust compiler's target feature and considering the pattern, those intrinsics would naturally require h and/or svinval through #[target_feature] before stabilization.
My hypothesis is that, some have interests to make a program for a non-U-mode (e.g. S-mode OS/hypervisor and M-mode firmware) RISC-V environment and others do not (making those intrinsics in an inconsistent state).

It looks like llvm does support these features https://godbolt.org/z/hbdvbPY65, so we can just add them to rust, and then explicitly pushing/popping the environment in the assembly should not be needed. That does mean that the target feature must go through the standard process for stabilization, yes.

My hypothesis is that, some have interests to make a program for a non-U-mode (e.g. S-mode OS/hypervisor and M-mode firmware) RISC-V environment and others do not (making those intrinsics in an inconsistent state).

My knowledge of RISC-V is insufficient to understand the implications here. From my perspective, if an instruction requires a target feature, it should be marked as such, otherwise we're introducing holes in safe rust.

I agree that.

To note, I once strongly objected to adding supervisor/hypervisor extensions like h and svinval on rust-lang/rust#145076 but that the main reason I did that was because the proposal was to add a target triple for user-mode Linux development (RVA23U64). So, if someone's interested in the kernel-mode development on Rust + RISC-V, adding target features itself should not be a problem.

Unfortunately, I'm not interested in such use cases for now and I won't make a PR to add h and svinval just for that (I think we first need to investigate what is really needed to add Rust + RISC-V + kernel mode support).

I'm going to minimize this PR to contain pause only.

Unfortunately, I'm not interested in such use cases for now and I won't make a PR to add h and svinval just for that (I think we first need to investigate what is really needed to add Rust + RISC-V + kernel mode support).

Again I can't really judge the situation, but so long as the "h" extension is rooted in some sort of specification (and it seems to be), then adding a rust target feature that just defers to the LLVM target feature of the same name is fine.

We should not stabilize these intrinsics until the target feature situation is resolved, because adding a target feature later is a breaking change.

So, I think the target features should be added. I'd be happy to do that though if you don't feel like it.

So, I think the target features should be added. I'd be happy to do that though if you don't feel like it.

Okay, that's reasonable. Still, I'd like to split this PR to pause and others (as a follow-up after Rust compiler changes regarding h and svinval).

EDIT: Considering your comment, I will need to add a comment why #[target_feature] is not added to the pause intrinsic.

Split the original PR into two (now this is the part 1; pause only) and added a comment:

// Use `.option` directives to expose this HINT instruction
// (no-op if not supported by the hardware) without `#[target_feature]`.