Add RISC-V platform and PAUSE instruction #1262
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon. Please see the contribution instructions for more information. |
|
Thank you for your contribution!
Please split it in two commits.
It is not unsafe for that reason and probably it can be considered safe (see the reasoning of safe/unsafe in wasm).
Once we use the right version of llvm it should work with |
crates/core_arch/src/riscv/mod.rs
Outdated
| /// The PAUSE instruction is a HINT that indicates the current hart's rate of instruction retirement | ||
| /// should be temporarily reduced or paused. The duration of its effect must be bounded and may be zero. | ||
| #[inline(always)] | ||
| pub unsafe fn pause() { |
There was a problem hiding this comment.
You should add a feature that maps to the extension, see what it is done for the other arch extensions.
There was a problem hiding this comment.
Yes, adding feature gate to extension specific instructions are necessary. For pause instruction, this instruction is in HINT space (in I set which must be implemented by RISC-V cores), whether there is Zihintpause or not does not change architectural state after instruction is retired. It will always run on all RISC-V cores. I don't know if it's still required to add feature gate to this function, or should we write it down in documentation for clarification. :)
There was a problem hiding this comment.
You may add a comment about it.
I assume nothing else would use the same value so either it is ignored or it is used as intended and would not change in unexpected ways the behavior in the future.
Can you confirm that I understood correctly?
crates/core_arch/src/riscv/mod.rs
Outdated
| /// should be temporarily reduced or paused. The duration of its effect must be bounded and may be zero. | ||
| #[inline(always)] | ||
| pub fn pause() { | ||
| unsafe { asm!(".word 0x0100000F") } |
There was a problem hiding this comment.
Can you set the nomem and nostack options? Also does LLVM have an intrinsic for this instruction?
There was a problem hiding this comment.
Hello! I'll perform changes soon. BTW, I checked the LLVM source here (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/IntrinsicsRISCV.td), but I didn't find LLVM intrinsic for RISC-V pause instruction.
There was a problem hiding this comment.
There is a PR for adding the pause instruction in inline asm. It seems to have been accepted but not yet merged for over a year: https://reviews.llvm.org/D93019
crates/core_arch/src/riscv/mod.rs
Outdated
| /// | ||
| /// The PAUSE instruction is a HINT that indicates the current hart's rate of instruction retirement | ||
| /// should be temporarily reduced or paused. The duration of its effect must be bounded and may be zero. | ||
| #[inline(always)] |
There was a problem hiding this comment.
Is there a specific reason for using #[inline(always)]. A normal #[inline] should be sufficient.
There was a problem hiding this comment.
Hello! I suspect if a single instruction wrap would gain better performance if it inlines. Yes, for some debug purposes we should not inline these functions. and #[inline] only would behave better in these scenarios.
|
Other than that, LGTM. |
Use `#[inline]` instead of `#[inline(always)]`
|
This PR is still marked as a draft, is there anything else to do? |
|
@Amanieu The instruction is encoded as |
…anieu Add spin_loop hint for RISC-V architecture This commit uses the PAUSE instruction (rust-lang/stdarch#1262) to implement RISC-V spin loop, and updates `stdarch` submodule to use the merged PAUSE instruction.
…anieu Add spin_loop hint for RISC-V architecture This commit uses the PAUSE instruction (rust-lang/stdarch#1262) to implement RISC-V spin loop, and updates `stdarch` submodule to use the merged PAUSE instruction.
…anieu Add spin_loop hint for RISC-V architecture This commit uses the PAUSE instruction (rust-lang/stdarch#1262) to implement RISC-V spin loop, and updates `stdarch` submodule to use the merged PAUSE instruction.
…anieu Add spin_loop hint for RISC-V architecture This commit uses the PAUSE instruction (rust-lang/stdarch#1262) to implement RISC-V spin loop, and updates `stdarch` submodule to use the merged PAUSE instruction.
…anieu Add spin_loop hint for RISC-V architecture This commit uses the PAUSE instruction (rust-lang/stdarch#1262) to implement RISC-V spin loop, and updates `stdarch` submodule to use the merged PAUSE instruction.
…anieu Add spin_loop hint for RISC-V architecture This commit uses the PAUSE instruction (rust-lang/stdarch#1262) to implement RISC-V spin loop, and updates `stdarch` submodule to use the merged PAUSE instruction.
This pull request contains two parts: new RISC-V platform
core::archpackage, and a first platform specific instruction wrapperpause()function on RISC-V platform.As is defined in RISC-V Unprivileged Specification version 20191214-draft (December 1, 2021, with Zihintpause 2.0 Ratified), the PAUSE instruction is defined as follows:
This instruction is useful when we need to implement
core::hint::spin_loop()function for RISC-V platforms.The function
pause()is defined unsafe because caller must ensure it's called on RISC-V platforms. As is defined in the Specification, implementation of the corresponding pause instruction varies between platforms, it may be spin loop hint, a no-op or others, but will never throw illegal instruction exception. This instruction does not perform as a memory barrier.For using
.word: thepausepseudoinstruction would be supported after this LLVM review: D93019, by now (if I am correct) I can't write this instruction in current version Rust'sasm!macro. In case if I'm incorrect or this way is inappropriate, I'd submit this pull request as a draft.