-
Notifications
You must be signed in to change notification settings - Fork 13.3k
RISC-V target feature B implies Zba, Zbb, Zbs #139748
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
Conversation
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
This comment has been minimized.
This comment has been minimized.
zba, zbb, and zbs are already stable, B is a convenient alias for all three, so it seems reasonable to stabilize it as well. The fourth bitmanip extension, Zbc, is not implied by B.
27a8a9c
to
88d1dcf
Compare
r? compiler |
who are the RISC-V maintainers? |
I'm not sure if I'm looking in the right place, but I see these files list different sets of people: https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/platform-support/riscv32-unknown-none-elf.md#target-maintainers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This is what LLVM does too. https://github.com/llvm/llvm-project/blob/305006179341d8f657b89fdc7d76502f9bc5f0aa/llvm/lib/Target/RISCV/RISCVFeatures.td#L497
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to check for LLVM compatibility similar to 62612af#diff-9f3bfd2fdc228dcb1b352082a7ebc39b19c8bea15b73141f72d9f5021aa3c66cR266-R268?
It looks like FeatureStdExtB exists in rust-lang's fork of LLVM starting from LLVM 19: https://github.com/rust-lang/llvm-project/blob/rustc/19.1-2024-07-30/llvm/lib/Target/RISCV/RISCVFeatures.td And it seems the minimum LLVM version was raised to 19 last week: 12167d7 So I think this should be good as is (the compat checks in 62612af also seem to have been removed) |
I noticed this comment says we may need t-lang, but I'm not too familiar with procedures yet. Is this a bors command? =) https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/target_features.rs#L125 |
#139440 also adds the |
zba, zbb, and zbs are already stable, B is a convenient alias for all three, so it seems reasonable to stabilize it as well.
This feature name is supported by our current LLVM version (and LLVM uses the same name).
The fourth bitmanip extension, Zbc, is not implied by B.
For reference, the RISC-V unprivileged manual defines this in chapter 29 on the B extension: https://github.com/riscv/riscv-isa-manual/releases