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.

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

Add support for RISC-V relax target feature #109860

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Conversation

zyedidia
Copy link
Contributor

@zyedidia zyedidia commented Apr 2, 2023

This adds relax as an allowed RISC-V target feature. The relax feature in LLVM enables linker relaxation, an optimization specific to RISC-V that allows global variable accesses to be resolved by the linker by using the global pointer (gp) register (rather than constructing the addresses from scratch for each access). Enabling relax will cause LLVM to emit relocations in the object file that support this. The feature can be enabled in rustc with -C target-feature=+relax.

Currently this feature is disabled by default, but maybe it should be enabled by default since it is an easy performance improvement (but requires the gp register to be set up properly). GCC/Clang enable this feature by default (for both hosted/bare-metal targets), and include the -mno-relax flag to disable it (see here for the code that enables it in Clang). I think it would make sense to enable by default, at least for all hosted targets since the gp register should be automatically set up by the runtime. For bare-metal targets, gp must be set up manually, so it is probably best to leave off by default to avoid breaking existing applications that do not set up gp. Leaving it disabled by default for all targets is also reasonable though.

Let me know your thoughts. Thanks!

Fixes #109426.

@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 2, 2023
@luojia65
Copy link
Contributor

luojia65 commented Apr 8, 2023

For embedded and bare-metal RISC-V applications, we can send patches to them before changes in this pull request (+relax by default on embedded targets) is stablized. There aren't many of those open source applications yet; mainly github.com/rust-embedded projects and some real-time operating systems kernels. Embedded projects are limited in code size, they can benefit a lot from +relax by default.

@jackh726
Copy link
Member

jackh726 commented Apr 9, 2023

Not my area of expertise r? compiler

@rustbot rustbot assigned WaffleLapkin and unassigned jackh726 Apr 9, 2023
@WaffleLapkin
Copy link
Member

Me neither :(
r? compiler

@rustbot rustbot assigned petrochenkov and unassigned WaffleLapkin Apr 9, 2023
@petrochenkov
Copy link
Contributor

The feature is unstable, so it should be fine in any case.
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 10, 2023

📌 Commit 48be303 has been approved by petrochenkov

It is now in the queue for this repository.

@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 Apr 10, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2023
Add support for RISC-V relax target feature

This adds `relax` as an allowed RISC-V target feature. The relax feature in LLVM enables [linker relaxation](https://www.sifive.com/blog/all-aboard-part-3-linker-relaxation-in-riscv-toolchain), an optimization specific to RISC-V that allows global variable accesses to be resolved by the linker by using the global pointer (`gp`) register (rather than constructing the addresses from scratch for each access). Enabling `relax` will cause LLVM to emit relocations in the object file that support this. The feature can be enabled in rustc with `-C target-feature=+relax`.

Currently this feature is disabled by default, but maybe it should be enabled by default since it is an easy performance improvement (but requires the `gp` register to be set up properly). GCC/Clang enable this feature by default (for both hosted/bare-metal targets), and include the `-mno-relax` flag to disable it (see [here](https://github.com/llvm/llvm-project/blob/466d554dcab39c3d42fe0c5b588b795e0e4b9d0d/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L145) for the code that enables it in Clang). I think it would make sense to enable by default, at least for all hosted targets since the `gp` register should be automatically set up by the runtime. For bare-metal targets, `gp` must be set up manually, so it is probably best to leave off by default to avoid breaking existing applications that do not set up `gp`. Leaving it disabled by default for all targets is also reasonable though.

Let me know your thoughts. Thanks!

Fixes rust-lang#109426.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 11, 2023
Add support for RISC-V relax target feature

This adds `relax` as an allowed RISC-V target feature. The relax feature in LLVM enables [linker relaxation](https://www.sifive.com/blog/all-aboard-part-3-linker-relaxation-in-riscv-toolchain), an optimization specific to RISC-V that allows global variable accesses to be resolved by the linker by using the global pointer (`gp`) register (rather than constructing the addresses from scratch for each access). Enabling `relax` will cause LLVM to emit relocations in the object file that support this. The feature can be enabled in rustc with `-C target-feature=+relax`.

Currently this feature is disabled by default, but maybe it should be enabled by default since it is an easy performance improvement (but requires the `gp` register to be set up properly). GCC/Clang enable this feature by default (for both hosted/bare-metal targets), and include the `-mno-relax` flag to disable it (see [here](https://github.com/llvm/llvm-project/blob/466d554dcab39c3d42fe0c5b588b795e0e4b9d0d/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L145) for the code that enables it in Clang). I think it would make sense to enable by default, at least for all hosted targets since the `gp` register should be automatically set up by the runtime. For bare-metal targets, `gp` must be set up manually, so it is probably best to leave off by default to avoid breaking existing applications that do not set up `gp`. Leaving it disabled by default for all targets is also reasonable though.

Let me know your thoughts. Thanks!

Fixes rust-lang#109426.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2023
Rollup of 8 pull requests

Successful merges:

 - rust-lang#109527 (Set up standard library path substitution in rust-gdb and gdbgui)
 - rust-lang#109752 (Stall auto trait assembly in new solver for int/float vars)
 - rust-lang#109860 (Add support for RISC-V relax target feature)
 - rust-lang#109923 (Update `error [E0449]: unnecessary visibility qualifier` to be more clear)
 - rust-lang#110070 (The `wrapping_neg` example for unsigned types shouldn't use `i8`)
 - rust-lang#110146 (fix(doc): do not parse inline when output is json for external crate)
 - rust-lang#110147 (Add regression test for rust-lang#104916)
 - rust-lang#110149 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5af6385 into rust-lang:master Apr 11, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RISC-V LLVM feature +relax not recognized by rustc
7 participants