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.

Sign up for GitHub

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

Jump to bottom

Add spin_loop hint for RISC-V architecture #91548

Merged
merged 2 commits into from
Dec 11, 2021
Merged

Add spin_loop hint for RISC-V architecture #91548

merged 2 commits into from
Dec 11, 2021

Conversation

luojia65
Copy link
Contributor

@luojia65 luojia65 commented Dec 5, 2021 •
edited
Loading

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.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2021
@luojia65
Add spin_loop hint for RISC-V architecture
70855b2 
This commit also updates `stdarch` git submodule.
@luojia65 luojia65 force-pushed the hint-spin-loop-riscv branch from 6293c09 to 70855b2 Compare December 5, 2021 08:39
@Disasm
Copy link
Contributor

Disasm commented Dec 6, 2021

spin_loop is also used internally (for example, in Arc::downgrade), so probably it's a bad idea to implement this function with the instruction that is part of the (optional) architecture extension.

@luojia65
Copy link
Contributor Author

luojia65 commented Dec 7, 2021 •
edited
Loading

@Disasm Pause is in RISC-V I's hint space. It's unlike other architectures, although this extension is optional, these hint instructions will still run (but effectively as an no-op) on unsupported platforms (other architectures may throw illegal instruction). This design may make it useful on standard library functions for all platforms if I am correct ;)

@Disasm
Copy link
Contributor

Disasm commented Dec 7, 2021

although this extension is optional, these hint instructions will still run

@luojia65 where can I read more about it? It's not obvious that this instruction is guaranteed to work even if not supported.

@luojia65
Copy link
Contributor Author

luojia65 commented Dec 7, 2021

@Disasm in this file if I am correct: https://github.com/riscv/riscv-isa-manual/blob/5cc0aa8d84464a4eee71713c5566d64cf3c4bdda/src/zihintpause.tex#L40
definition for hint space: https://github.com/riscv/riscv-isa-manual/blob/5cc0aa8d84464a4eee71713c5566d64cf3c4bdda/src/rv32.tex#L1403

@Mark-Simulacrum
Copy link
Member

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 8, 2021

📌 Commit 70855b2 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 8, 2021
@matthiaskrgr
Rollup merge of rust-lang#91548 - luojia65:hint-spin-loop-riscv, r=Am…
9e39487 
…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.
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 8, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 8, 2021
@matthiaskrgr
Rollup merge of rust-lang#91548 - luojia65:hint-spin-loop-riscv, r=Am…
22afb94 
…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.
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 8, 2021
Closed
@matthiaskrgr
Copy link
Member

Probably caused this rollup failure, please check: #91658 (comment)
@bors r- rollup=iffy

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 8, 2021
This was referenced Dec 9, 2021
Merged
Merged
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 9, 2021
@luojia65
Copy link
Contributor Author

luojia65 commented Dec 9, 2021 •
edited
Loading

Hello! I fixed probable rollup error in rust-lang/stdarch#1264, and updated submodule to include this fix. Now this pull request may build with CI passed.

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Dec 9, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2021

📌 Commit 0ccf58b has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 9, 2021
@matthiaskrgr
Rollup merge of rust-lang#91548 - luojia65:hint-spin-loop-riscv, r=Am…
27974b9 
…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.
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 9, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 11, 2021
@matthiaskrgr
Rollup merge of rust-lang#91548 - luojia65:hint-spin-loop-riscv, r=Am…
1305bc5 
…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.
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 11, 2021
@matthiaskrgr
Rollup merge of rust-lang#91548 - luojia65:hint-spin-loop-riscv, r=Am…
2356936 
…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.
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2021
@bors
Auto merge of rust-lang#91799 - matthiaskrgr:rollup-b38xx6i, r=matthi…
928783d 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#83174 (Suggest using a temporary variable to fix borrowck errors)
 - rust-lang#89734 (Point at capture points for non-`'static` reference crossing a `yield` point)
 - rust-lang#90270 (Make `Borrow` and `BorrowMut` impls `const`)
 - rust-lang#90741 (Const `Option::cloned`)
 - rust-lang#91548 (Add spin_loop hint for RISC-V architecture)
 - rust-lang#91721 (Minor improvements to `future::join!`'s implementation)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 60b9f31 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
@petrochenkov petrochenkov mentioned this pull request Dec 12, 2021
Closed
@luojia65 luojia65 deleted the hint-spin-loop-riscv branch December 22, 2021 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Amanieu Amanieu

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
9 participants
@luojia65 @rust-highfive @Disasm @Mark-Simulacrum @Amanieu @bors @matthiaskrgr @apiraino @rustbot