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 · 13 comments
Open

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

workingjubilee opened this issue Nov 4, 2024 · 13 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.

@RalfJung
Copy link
Member

Targets using any ABI other than ilp32e/lp64e rely on the e feature being disabled.

Wait so enabling e actually makes fewer things work? 😢 and here I was thinking RISC-V actually was a sane target... the float ABI stuff in RISC-V actually makes sense unlike in any other target I looked at so far, but having negative features is a nightmare.

@beetrees
Copy link
Contributor

beetrees commented Dec 16, 2024

Looking at the RISC-V ISA specification, the RV32E/RV64E base instruction sets are described as reduced versions of RV32I/RV64I (the only difference being fewer available GPR registers): this seems similar to the way that e.g. the Zfhmin extension is a subset of the Zfh extension. So rather than having e as a negative feature, I think it would make more sense to have an i feature which is enabled when targeting RV32I/RV64I and disabled when targeting RV32E/RV64E. This allow things like #[target_feature(enable = "i")] to work as expected, and would be much more consistent.

@workingjubilee
Copy link
Member Author

workingjubilee commented Dec 16, 2024

Targets using any ABI other than ilp32e/lp64e rely on the e feature being disabled.

Wait so enabling e actually makes fewer things work? 😢 and here I was thinking RISC-V actually was a sane target... the float ABI stuff in RISC-V actually makes sense unlike in any other target I looked at so far, but having negative features is a nightmare.

technically the way it actually works is that the architectures are, from the high-level perspective,

  • rv32i
  • rv64i
  • rv128i(?)
  • rv32e
  • rv64e

i.e. LLVM representing it as a feature is on LLVM, and from the RISCV architectural definition perspective these are all separate "base" architectures.

@workingjubilee
Copy link
Member Author

relevant to this in a slightly sideways manner: #134358 will expose a way to cfg on RV32E targets without requiring a specific disposition for which way we zig or zag between -e or +i

@RalfJung
Copy link
Member

RalfJung commented Dec 16, 2024

Looking at the RISC-V ISA specification, the RV32E/RV64E base instruction sets are described as reduced versions of RV32I/RV64I (the only difference being fewer available GPR registers): this seems similar to the way that e.g. the Zfhmin extension is a subset of the Zfh extension. So rather than having e as a negative feature, I think it would make more sense to have an i feature which is enabled when targeting RV32I/RV64I and disabled when targeting RV32E/RV64E. This allow things like #[target_feature(enable = "i")] to work as expected, and would be much more consistent.

Yeah that makes sense, but it would divert from how RISCV target features are handled in other languages. Seems like a discussion for #114544.

i.e. LLVM representing it as a feature is on LLVM, and from the RISCV architectural definition perspective these are all separate "base" architectures.

Oh wait this is an LLVM thing after all? What does GCC do?

If it's actually a different architecture then maybe marking it as "forbidden" (i.e. a target feature that is fixed by the target spec) isn't the worst idea?

@RalfJung
Copy link
Member

#134337 catches most of the potential ABI issues listed above. What remains are problems caused by feature implications that rustc does not know about. I have some plans here but that will have to wait for a future PR.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 16, 2024
…workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc `@beetrees` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? `@workingjubilee`

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 16, 2024
…workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc ``@beetrees`` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? ``@workingjubilee``

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 16, 2024
Rollup merge of rust-lang#134337 - RalfJung:riscv-target-features, r=workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment)))
Part of rust-lang#116344

Cc ``@beetrees`` I hope I got everything.  I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? ``@workingjubilee``

Ideally we'd also reject target specs that disable the `f` feature but set an ABI that requires `f`... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.
@workingjubilee
Copy link
Member Author

I'm not sure gcc doesn't do the same thing, I'm addressing "how the specification for RISCV itself reasons about the underlying thing going on here". Essentially, the answer from the hardware architects might be that there are really 4 different target_arch values:

  • "riscv32i"
  • "riscv64i"
  • "riscv32e"
  • "riscv64e"

It seems doubtful we would want to actually go that route, but nonetheless!

@RalfJung
Copy link
Member

So... is it even worth supporting toggling the "e" feature then? We don't support toggling the target architecture, either...

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

5 participants