Conversation
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| #[inline] | ||
| #[target_feature(enable = "lasx")] | ||
| #[unstable(feature = "stdarch_loongarch", issue = "117427")] | ||
| #[stable(feature = "stdarch_loongarch_simd", since = "CURRENT_RUSTC_VERSION")] | ||
| pub fn lasx_xvsrar_b(a: m256i, b: m256i) -> m256i { | ||
| unsafe { transmute(__lasx_xvsrar_b(transmute(a), transmute(b))) } | ||
| } |
There was a problem hiding this comment.
What's the reason for changing the feature name?
There was a problem hiding this comment.
What's the reason for changing the feature name?
There are also some unstable APIs gated by stdarch_loongarch. Having both stable and unstable APIs gated by the same feature name causes the following error:
error[E0711]: feature `stdarch_loongarch` is declared stable, but was previously declared unstable
Also, putting the SIMD intrinsics under stdarch_loongarch_simd makes the categorization more explicit, similar to stdarch_loongarch_crc.
|
Note that this stabilizes more cases of Nominating since that's probably worth clarifying, and also this is a lot of surface area with potential sharp edges. @rustbot label +I-libs-nominated |
|
Error: The feature Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
Thanks for pointing this out. As far as I know, no projects currently depend on the LoongArch SIMD API signatures using the |
5f1c38d to
bc04e14
Compare
|
|
|
@rfcbot merge libs |
|
@clarfonthey has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
It would be nice to have this fuzz tested against the C intrinsics using intrinsic-test before stabilizing. @rfcbot concern fuzz testing |
|
There was a previous attempt at doing so at #1900, though it never fully passed CI and will need some rebasing. |
|
With these intrinsics, Miri reports UB in safe code: #![feature(stdarch_loongarch)]
use std::arch::is_loongarch_feature_detected;
use std::arch::loongarch64::*;
#[target_feature(enable = "lsx")]
fn probe() -> i32 {
let ones = lsx_vrepli_b::<1>();
let zero = lsx_vrepli_b::<0>();
let r = lsx_vdiv_b(ones, zero);
lsx_vpickve2gr_b::<0>(r)
}
fn main() {
if is_loongarch_feature_detected!("lsx") {
// SAFETY: We just checked: the `lsx` feature is present.
let lane0 = unsafe { probe() };
println!("lane 0 = {lane0}");
}
}From #2078, see:
cc @sayantn |
Good catch! I double-checked this with the LoongArch ISA developer. It differs from the previous comment in PR #2078. The ISA defines So it seems we can't use portable |
|
There are 2 options, depending on what the intrinsic behavior should be:
|
Thanks! A new PR has been created based on option 2: #2229 |
Miri reports UB on division by zero with the LSX vdiv/vmod intrinsics when implemented via the portable simd_div/simd_rem helpers. [^1] Per clarification from the LoongArch ISA developers, the hardware defines that vdiv/vmod return 0 when the divisor is zero (this will be documented in the SIMD reference). This differs from the earlier assumption in PR rust-lang#2078, so the portable helpers are not a correct match. Switch back to the corresponding LLVM intrinsics so the Rust wrappers preserve the defined ISA behavior. [^1]: rust-lang#2221 (comment)
Tracking issue: rust-lang/rust#162508
LoongArch64 SIMD intrinsics are already used by rustc itself and by a number of downstream projects. Experience from these real-world uses indicates that the API is ready for stabilization. Stabilizing the intrinsics would remove the nightly requirement for applications that currently depend on them.
API surface
Note for subtree update
Remove this line: https://github.com/rust-lang/rust/blob/1edd55dcfcd573872c727fa3e086369a71661ee0/compiler/rustc_span/src/lib.rs#L20