Uh oh!
There was an error while loading. Please reload this page.
ABI checks: add support for loongarch - #133249
Conversation
This comment has been minimized.
This comment has been minimized.
workingjubilee
commented
Nov 20, 2024
lsx? lasx? oh dear. |
heiher
commented
Nov 20, 2024
LSX is a 128-bit vector extension, and LASX is a 256-bit vector extension. Parameters and return values for neither of these data types are passed through vector registers. 😃 |
workingjubilee
commented
Nov 20, 2024
Then why is the lint triggering? |
workingjubilee
commented
Nov 20, 2024
Here is the It looks fairly dubious to me. https://github.com/rust-lang/rust/blame/bfe809d93c67de03e3a84b80549927fd828ec5d0/compiler/rustc_monomorphize/src/mono_checks/abi_check.rs#L43 |
workingjubilee
commented
Nov 20, 2024
This is not the correct fix, the correct fix is to fix the lint itself. |
I see it. But I'm not sure how to make it more accurate. Here's a case: #![feature(stdarch_loongarch)]use std::arch::loongarch64::*;pubunsafefnsimd(s:i32) -> u32{lsx_vpickve2gr_bu::<0>(lsx_vreplgr2vr_b(s))}and the PassMode is BackendRepr is |
heiher
commented
Nov 21, 2024
Is it true that the Rust frontend cannot determine whether a parameter is passed through a vector register or a general-purpose register based solely on |
workingjubilee
commented
Nov 21, 2024
My understanding is the frontend technically cannot precisely control it, no. |
heiher
commented
Nov 25, 2024
Since we cannot determine the type of registers used for vectors in Rust's front-end, I have reverted the return type of Do you think this is ok? @workingjubilee |
workingjubilee
commented
Nov 25, 2024
Hmm. |
workingjubilee
commented
Nov 25, 2024
What we want is to both
If the backend does not expose some ability to handle register usage (which does imply that we should have a nonzero ability to get it wrong), then I think we will simply fuck things up in a different way from our current approaches. |
In either version of this PR that I have seen so far, it adjusts things in a way that fixes things for the C ABI in certain conditions. However, the motivation1 for the changes are that our current compiler contract with the Loongarch backend remains poorly-specified and poorly-understood.
So, before I merge any fixes, I would like to understand, relative to how the Loongarch backend handles the LLVMIR lowering,
Footnotes
|
heiher
commented
Nov 28, 2024
Okay. the following examples and LLVM IR shows how vector types are passed in the C ABI and Rust non-C ABI. LSX (128-bit vectors)
#![feature(stdarch_loongarch)]#![feature(loongarch_target_feature)]use std::arch::loongarch64::*;use std::mem::transmute;extern"C"fnno_target_feature(_dummy:f32,x:v4i32){let val = unsafe{transmute::<_,[u32;4]>(x)};dbg!(val);}#[target_feature(enable = "lsx")]unsafefnwith_target_feature(x:v4i32){no_target_feature(0.0, x);}fnmain(){unsafe{with_target_feature(transmute([1;4]));}}LLVM IR: ; lsx::no_target_feature; Function Attrs: nounwind uwtabledefineinternalvoid@_ZN3lsx17no_target_feature17h2cf7e0ecaa1b600aE(float%0, i128%1) unnamed_addr #3personalityptr@rust_eh_personality {
; ^--- by-val (i64, i64); ...
}
; lsx::with_target_feature; Function Attrs: uwtabledefineinternalvoid@_ZN3lsx19with_target_feature17hfda3b7c2fa82d5f7E(ptralign16%x) unnamed_addr #4 {
; ^--- by-ref; ...
}LASX (256-bit vectors)
#![feature(stdarch_loongarch)]#![feature(loongarch_target_feature)]use std::arch::loongarch64::*;use std::mem::transmute;extern"C"fnno_target_feature(_dummy:f32,x:v8i32){let val = unsafe{transmute::<_,[u32;8]>(x)};dbg!(val);}#[target_feature(enable = "lasx")]unsafefnwith_target_feature(x:v8i32){no_target_feature(0.0, x);}fnmain(){unsafe{with_target_feature(transmute([1;8]));}}; lasx::no_target_feature; Function Attrs: nounwind uwtabledefineinternalvoid@_ZN4lasx17no_target_feature17h94e0853bc1ae88d5E(float%0, ptralign32%x) unnamed_addr #3personalityptr@rust_eh_personality {
; ^--- by-ref; ...
}
; lasx::with_target_feature; Function Attrs: uwtabledefineinternalvoid@_ZN4lasx19with_target_feature17h6ea2f98ed3995f8fE(ptralign32%x) unnamed_addr #4 {
; ^--- by-ref; ...
}Vector registers are not used for parameter passing, and to maintain ABI compatibility, the calling convention will not be changed in the foreseeable future. That's why I marked LoongArch as not requiring vector ABI checks. |
On some targets, the parameter passing conventions are controlled by the target features. If the caller and callee have different target features and the data type being passed is dependent on these features, inconsistencies can arise. The purpose of this ABI check is to detect such issues at compile-time. Technically, the parameter passing rules for vector data types in LoongArch are not affected by the target features, which applies to both C ABI and non-C ABI. For the first version, due to the limitation that the Rust frontend cannot determine the register type used for passing vector types, we made a concession and accepted this unnecessary ABI check. It does not cause any issues. For the second version, more technical analysis is required compared to the first. It is only feasible to proceed once it is confirmed that the parameter passing rules for LoongArch are not related to the target features. I would like to clarify that this is indeed the case for LoongArch. Is there anything else we can do? |
heiher
commented
Dec 10, 2024
@workingjubilee (a gentle ping) |
heiher
commented
Dec 11, 2024
I attempted once again to find a direct approach to make Regarding the indirect workaround, the first version appears to be preferable to the second. The second version bypasses the ABI check entirely for LoongArch, which means that if new vector data types are introduced in the future and indeed use vector registers, the ABI check would remain ineffective. In contrast, the first version aligns with the current design pattern. While the check might be redundant for the existing vector features, |
LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc
I see. I think I agree the first version seems preferable, now, however this has drawn my attention:
Can we use the |
heiher
commented
Dec 11, 2024
Thanks for your reply. I’ll give it a try and propose it in a new PR. However, would switching to |
@heiher There is no such thing as ABI compatibility between versions of the compiler for Rust. The Rust ABI code is only guaranteed compatibility for a single version of the compiler. |
workingjubilee
commented
Dec 12, 2024
And yes, handling the actual ABI change in a separate PR is fine. @bors r+ rollup |
bors
commented
Dec 12, 2024
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133122 (Add unpolished, experimental support for AFIDT (async fn in dyn trait)) - rust-lang#133249 (ABI checks: add support for loongarch) - rust-lang#134089 (Use newly added exceptions to non default branch warning) - rust-lang#134188 (Bump Fuchsia) - rust-lang#134204 (Fix our `llvm::Bool` typedef to be signed, to match `LLVMBool`) - rust-lang#134207 (Revert "bootstrap: print{ln}! -> eprint{ln}! (take 2) rust-lang#134040") - rust-lang#134214 (rustdoc: fix self cmp) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#133249 - heiher:loong-abi-check, r=workingjubilee ABI checks: add support for loongarch LoongArch psABI[^1] specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack. This patch addresses the following warnings: ``` warning: this function call uses a SIMD vector type that is not currently supported with the chosen ABI --> .../library/core/src/../../stdarch/crates/core_arch/src/loongarch64/lsx/generated.rs:3695:5 | 3695 | __lsx_vreplgr2vr_b(a) | ^^^^^^^^^^^^^^^^^^^^^ function called here | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue rust-lang#116558 <rust-lang#116558> = note: `#[warn(abi_unsupported_vector_types)]` on by default ``` [^1]: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc r? `@workingjubilee`
LoongArch psABI1 specifies that LSX vector types are passed via general-purpose registers, while LASX vector types are passed indirectly through the stack.
This patch addresses the following warnings:
r? @workingjubilee
Footnotes
https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc↩