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

Some -Ctarget-features must be restrained on RISCV #132618

Open
workingjubilee opened this issue Nov 4, 2024 · 5 comments
Open

Some -Ctarget-features must be restrained on RISCV #132618

workingjubilee opened this issue Nov 4, 2024 · 5 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Nov 4, 2024

refs:

in discussion of the target modifiers RFC1, Ralf asks this question:

That's actually fine on most targets, e.g. on x86 if you set the soft-float target feature then you can enable x87 and SSE and whatnot and keep using the soft-float ABI. Only aarch64 llvm/llvm-project#110632 (from the targets I checked so far).

The interesting question is what happens on RISC-V when I disable the FP instructions on a target that usually uses the hardfloat ABI. Sadly, what LLVM usually does in that case is silently fall back to the softfloat ABI. But I haven't checked for RISC-V.

cc @beetrees

Footnotes

  1. https://github.com/rust-lang/rfcs/pull/3716

@workingjubilee workingjubilee added A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status O-riscv Target: RISC-V architecture labels Nov 4, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 4, 2024
@workingjubilee workingjubilee changed the title Must some -Ctarget-features be restrained on RISCV targets? Must some -Ctarget-features be restrained on RISCV? Nov 4, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 5, 2024
@beetrees
Copy link
Contributor

beetrees commented Nov 5, 2024

In summary:

  • Targets using any ABI other than ilp32e/lp64e rely on the e feature being disabled.
  • Targets using the ilp32f/lp64f ABI rely on the f feature being enabled.
  • Targets using the ilp32d/lp64d ABI rely on the d feature being enabled.
  • (LLVM doesn't support the q feature yet, but a similar rule will apply with that feature and the lp64q ABI).
  • The forced-atomics feature must never be disabled if it is enabled, as the forced-atomics feature changes the ABI of atomic operations (as mentioned in this section of the LLVM atomics documentation).

Notes:

  • Currently LLVM will emit a warning and fallback to an inferred-from-target-features ABI if the required target features are disabled (or required-to-be-disabled target features are enabled).
  • Enabling the f/d features on targets using softer ABIs is fine, as is disabling the e feature, as all RISC-V targets explicitly state their ABI due to Always specify llvm_abiname for RISC-V targets #131807.
  • Disabling the a feature causes atomic operations to be compiled using __atomic_* libcalls instead of native operations, but AFAICT the worst possible outcome here is a linker error if libatomic is not linked - according to the LLVM atomics documentation, __atomic_* libcalls must use a lock-free implementation for all atomic sizes that the compiler can emit native instructions for (which are the only sizes of atomics Rust supports).

@workingjubilee
Copy link
Member Author

workingjubilee commented Nov 5, 2024

cc @coastalwhite (as author of #116485 which got scaled back due to precisely this question!)

@beetrees
Copy link
Contributor

beetrees commented Nov 5, 2024

Just realised I missed the e feature from my summary; I've edited my comment above to include it.

@beetrees
Copy link
Contributor

beetrees commented Nov 5, 2024

Also I haven't included above the cases where LLVM will emit an error itself due to feature incompatibility. Checking the features mentioned #116485, those are:

  • The f and zfinx features are incompatible.
  • The d feature is incompatible with the ilp32e ABI.

In both these cases LLVM will already emit a fatal error, but it would definitely be better diagnostic UX for rustc to detect the problem and emit an error itself.

@workingjubilee workingjubilee changed the title Must some -Ctarget-features be restrained on RISCV? Some -Ctarget-features must be restrained on RISCV Nov 8, 2024
@workingjubilee
Copy link
Member Author

workingjubilee commented Nov 8, 2024

Thanks for the summary! That seems pretty definite in terms of a "yes, we should", yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants