Repository navigation
Fix RISC-V inline ASM clobber_abi when extra target features are enabled - #163833
Conversation
|
r? @mu001999 rustbot has assigned @mu001999. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
e0c4734 to
4c968c7
Compare
| Ok(if matches!(target.llvm_abiname, LlvmAbi::Ilp32e | LlvmAbi::Lp64e) { | ||
| InlineAsmClobberAbi::RiscVE | ||
| } else { | ||
| InlineAsmClobberAbi::RiscV | ||
| }) |
There was a problem hiding this comment.
So if "e" gets disabled on these targets, we should still use the RiscVE clobbers?
There was a problem hiding this comment.
Yes. The ilp32e/lp64e ABIs clobber different registers from the other ABIs (specifically, x18-x27 are clobbered by ilp32e/lp64e but not be ilp32/lp64).
| // Only ABI_FLEN bits of the callee-saved floating point registers are | ||
| // preserved. As LLVM doesn't have partial clobbers, this means we have to | ||
| // clobber the callee-saved floating point registers as well as the temporary | ||
| // registers. |
There was a problem hiding this comment.
Could we instead have 3 variants of the clobber ABI:
RiscVEwhere upper X registers and all FP registers are clobberedRiscVwhere all FP regsiters are clobbered.RiscVDwhere only caller-saved FP registers are clobbered.
There was a problem hiding this comment.
Done, although it will need updating once support for the q extension is added to LLVM.
There was a problem hiding this comment.
Would it make sense to add that ABI to the enum to bug! if it gets hit and we forget to update?
There was a problem hiding this comment.
It's not the lp64q ABI that would cause the issue: LLVM understanding how to use the 128-bit FLEN from the q target feature and the q feature being enabled (either via -Ctarget-feature/-Zllvm-target-feature or in C code that has the feature enabled and is LTOed to the Rust code) would make the ABI_FLEN==FLEN assumption invalid. LLVM currently only has assembler support for q, so this is not yet an issue.
4c968c7 to
94d4660
Compare
|
r? Amanieu |
|
r=me once #163839 lands and this is rebased |
This comment has been minimized.
This comment has been minimized.
94d4660 to
930372b
Compare
|
@bors r=Amanieu rollup |
Rollup of 10 pull requests Successful merges: - #160186 (Implement `Fn` traits for safe functions of any ABI) - #160370 (rustdoc: notable trait badge and popover enhancements) - #165077 (Fix alloctests for panic=abort) - #163623 (Make docblock `<code>` elements inline so they behave better) - #163833 (Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled) - #165067 (cg_llvm: Use an invariant lifetime brand for LLVM modules ) - #165227 (Respect `--color=never` when printing `--explain` output to the terminal) - #165254 (Cleanup `rustdoc-html` tests) - #165258 (impl Eq for AllocRange) - #165259 (rustc-dev-guide subtree update)
Rollup merge of #163833 - beetrees:fix-riscv-clobber-abi, r=Amanieu Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled Waiting on #163839, a beta-backportable subset of this PR. This PR fixes several issues with `clobber_abi("C")` on RISC-V: - Whether to clobber the registers for `ilp32e`/`lp64e` or not was previously decided based on whether the `e` target feature was enabled, as opposed to the actual ABI in use. - `ilp32e`/`lp64e` failed to clobber all floating point registers and the upper integer registers as specified in the ABI specification. As a side effect of fixing this, `x16`-`x31` can now be used as clobbers when the `e` feature is enabled (as RISC-V inline ASM is stable, I think this might require a FCP?). - Callee-saved floating point registers are only callee-saved up to ABI_FLEN bits; the rest of the register is considered temporary. As LLVM doesn't support partial register clobbers, this requires the callee-saved floating point registers to be clobbered.
Rollup of 10 pull requests Successful merges: - rust-lang/rust#160186 (Implement `Fn` traits for safe functions of any ABI) - rust-lang/rust#160370 (rustdoc: notable trait badge and popover enhancements) - rust-lang/rust#165077 (Fix alloctests for panic=abort) - rust-lang/rust#163623 (Make docblock `<code>` elements inline so they behave better) - rust-lang/rust#163833 (Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled) - rust-lang/rust#165067 (cg_llvm: Use an invariant lifetime brand for LLVM modules ) - rust-lang/rust#165227 (Respect `--color=never` when printing `--explain` output to the terminal) - rust-lang/rust#165254 (Cleanup `rustdoc-html` tests) - rust-lang/rust#165258 (impl Eq for AllocRange) - rust-lang/rust#165259 (rustc-dev-guide subtree update)
Rollup of 10 pull requests Successful merges: - rust-lang/rust#160186 (Implement `Fn` traits for safe functions of any ABI) - rust-lang/rust#160370 (rustdoc: notable trait badge and popover enhancements) - rust-lang/rust#165077 (Fix alloctests for panic=abort) - rust-lang/rust#163623 (Make docblock `<code>` elements inline so they behave better) - rust-lang/rust#163833 (Fix RISC-V inline ASM `clobber_abi` when extra target features are enabled) - rust-lang/rust#165067 (cg_llvm: Use an invariant lifetime brand for LLVM modules ) - rust-lang/rust#165227 (Respect `--color=never` when printing `--explain` output to the terminal) - rust-lang/rust#165254 (Cleanup `rustdoc-html` tests) - rust-lang/rust#165258 (impl Eq for AllocRange) - rust-lang/rust#165259 (rustc-dev-guide subtree update)
Waiting on #163839, a beta-backportable subset of this PR.
This PR fixes several issues with
clobber_abi("C")on RISC-V:ilp32e/lp64eor not was previously decided based on whether theetarget feature was enabled, as opposed to the actual ABI in use.ilp32e/lp64efailed to clobber all floating point registers and the upper integer registers as specified in the ABI specification. As a side effect of fixing this,x16-x31can now be used as clobbers when theefeature is enabled (as RISC-V inline ASM is stable, I think this might require a FCP?).