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

RISC-V LLVM feature +relax not recognized by rustc #109426

Closed
zyedidia opened this issue Mar 21, 2023 · 6 comments · Fixed by #109860
Closed

RISC-V LLVM feature +relax not recognized by rustc #109426

zyedidia opened this issue Mar 21, 2023 · 6 comments · Fixed by #109860
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@zyedidia
Copy link
Contributor

Code

// Compile with: rustc --target riscv64imac-unknown-none-elf -C target-feature=+relax test.rs --emit=obj

#![no_std]
#![no_main]

use core::panic::PanicInfo;

#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
    loop {}
}

Current output

warning: unknown feature specified for `-Ctarget-feature`: `relax`
  |
  = note: it is still passed through to the codegen backend
  = help: consider filing a feature request

warning: 1 warning emitted

Desired output

No warning.

Rationale and extra context

I would like to enable linker relaxation for the RISC-V target. To do so I passed -C target-feature=+relax to tell LLVM to use linker relaxation. It seems like rustc is not aware of this option, so it emits a warning (but the feature is still properly enabled). I can't find any way to disable this warning, so I am submitting this issue in the hopes that either the feature will become recognized, or a way to disable the warning will be added.

Related: #96472.

The program is compiled by running rustc --target riscv64imac-unknown-none-elf -C target-feature=+relax test.rs --emit=obj

Hopefully I've posted this to the correct place. Thanks!

Other cases

No response

Anything else?

No response
@zyedidia zyedidia added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 21, 2023
@nikic
Copy link
Contributor

nikic commented Mar 21, 2023

Does -Z relax-elf-relocations work in this context?

@zyedidia
Copy link
Contributor Author

I tried it out and it does not seem to enable linker relaxation unfortunately.

@yaoxin1995
Copy link

@nikic I wonder where could I add -Z relax-elf-relocations? Now I am using cargo build:

RUSTFLAGS="..." CARGO_TARGET_DIR=../target cargo xbuild --target x86_64-qkernel.json  --verbose

@workingjubilee
Copy link
Member

Why is a target feature being used for linker relaxation...?

@zyedidia
Copy link
Contributor Author

This optimization/feature is specific to RISC-V. For the RISC-V target by default LLVM does not emit relocations (in the generated object file) for global variable lookups that tell the linker to make use of linker relaxation (generating addresses for globals using the gp register). I’m not sure of the rationale behind this (gcc enables it by default and uses -mno-relax to disable it). One reason might be that using linker relaxation does require the runtime or something to set up the gp register beforehand.

In any case it should be possible to enable with Rust without getting a warning, and perhaps should even be enabled by default for RISC-V targets.

@workingjubilee
Copy link
Member

workingjubilee commented Mar 23, 2023 •
edited
Loading

As mentioned in the other issue, the warning is in large part because LLVM and Rust don't always agree on the definition of a target feature, in part because LLVM has proven surprisingly willing to change the definitions.

Proooobably we should just enable this by default, then, if gcc does? Hmm.

@zyedidia zyedidia mentioned this issue Apr 2, 2023
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 10, 2023
@Dylan-DPC
Rollup merge of rust-lang#109860 - zyedidia:riscv-relax, r=petrochenkov
083d5a8 
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:/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 issue Apr 11, 2023
@JohnTitor
Rollup merge of rust-lang#109860 - zyedidia:riscv-relax, r=petrochenkov
30458d1 
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:/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 bors closed this as completed in 5af6385 Apr 11, 2023
@SUPERCILEX SUPERCILEX mentioned this issue May 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for RISC-V relax target feature zyedidia/rust
4 participants
@nikic @zyedidia @workingjubilee @yaoxin1995