-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
simd_shuffle intrinsic: allow argument to be passed as vector #128731
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1279,19 +1279,24 @@ fn generic_simd_intrinsic<'ll, 'tcx>( | |
} | ||
|
||
if name == sym::simd_shuffle { | ||
// Make sure this is actually an array, since typeck only checks the length-suffixed | ||
// Make sure this is actually an array or SIMD vector, since typeck only checks the length-suffixed | ||
// version of this intrinsic. | ||
let n: u64 = match args[2].layout.ty.kind() { | ||
let idx_ty = args[2].layout.ty; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "index type"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the type of the argument named |
||
let n: u64 = match idx_ty.kind() { | ||
ty::Array(ty, len) if matches!(ty.kind(), ty::Uint(ty::UintTy::U32)) => { | ||
len.try_eval_target_usize(bx.cx.tcx, ty::ParamEnv::reveal_all()).unwrap_or_else( | ||
|| span_bug!(span, "could not evaluate shuffle index array length"), | ||
) | ||
} | ||
_ => return_error!(InvalidMonomorphization::SimdShuffle { | ||
span, | ||
name, | ||
ty: args[2].layout.ty | ||
}), | ||
_ if idx_ty.is_simd() | ||
&& matches!( | ||
idx_ty.simd_size_and_type(bx.cx.tcx).1.kind(), | ||
ty::Uint(ty::UintTy::U32) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we actually need to enforce signedness? Though the 32-bitness is certainly fixed... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We enforce it in the array version so I figured we should keep enforcing it in the vector version. |
||
) => | ||
{ | ||
idx_ty.simd_size_and_type(bx.cx.tcx).0 | ||
} | ||
_ => return_error!(InvalidMonomorphization::SimdShuffle { span, name, ty: idx_ty }), | ||
}; | ||
|
||
let (out_len, out_ty) = require_simd!(ret_ty, SimdReturn); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,15 +6,20 @@ | |
#![allow(incomplete_features)] | ||
#![feature(adt_const_params)] | ||
|
||
use std::marker::ConstParamTy; | ||
|
||
extern "rust-intrinsic" { | ||
fn simd_shuffle<T, I, U>(a: T, b: T, i: I) -> U; | ||
} | ||
Comment on lines
11
to
13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, can't we just, like, import this from core::intrinsics now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this test is older than that. |
||
|
||
#[derive(Copy, Clone)] | ||
#[derive(Copy, Clone, ConstParamTy, PartialEq, Eq)] | ||
#[repr(simd)] | ||
struct Simd<T, const N: usize>([T; N]); | ||
|
||
pub unsafe fn __shuffle_vector16<const IDX: [u32; 16], T, U>(x: T, y: T) -> U { | ||
unsafe fn __shuffle_vector16<const IDX: [u32; 16], T, U>(x: T, y: T) -> U { | ||
simd_shuffle(x, y, IDX) | ||
} | ||
unsafe fn __shuffle_vector16_v2<const IDX: Simd<u32, 16>, T, U>(x: T, y: T) -> U { | ||
simd_shuffle(x, y, IDX) | ||
} | ||
|
||
|
@@ -30,6 +35,17 @@ fn main() { | |
let y: Simd<u8, 2> = simd_shuffle(a, b, I2); | ||
assert_eq!(y.0, [1, 5]); | ||
} | ||
// Test that we can also use a SIMD vector instead of a normal array for the shuffle. | ||
const I1_SIMD: Simd<u32, 4> = Simd([0, 2, 4, 6]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually we avoid direct construction of Simd like this, but I suppose the fact that it occasionally emits illegal LLVMIR doesn't matter since this is const-evaluated. I suppose the other accesses and constructions here are preexisting, too, probably preceded that bug report, so not worth blocking this over since we should really try to scan the entire repo for them... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a soundness bug where directly constructing SIMD values can miscompile? We shouldn't work around that in code, we should get codegen fixed... Is this tracked anywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @scottmcm Can you refresh my memory on what the issue was here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Direct construction, as far as I know, is fine™. Both rust-lang/stdarch#1624 and (soon) #129403 have a bunch of it. We might ban projecting into a simd type at some point in the future, because as I recall it's awkward for codegen, which doesn't want the lanes to be places, because it wants to extract/insert them instead of addressing them. But that's not happened at this point, and is already in enough places that a few more is no big deal. And I think it might not even apply to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (And, come to think of it, it might not even be a problem for array-based simd, because using the array as a place forces it to memory anyway. It makes me wonder if the problem was more about just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Makes sense. However, if "awkward" leads to "generate wrong code" we should track that as an I-unsound issue somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know any current "wrong code" cases, just recall that cg_clif had to do a bunch of extra work to make it work. |
||
const I2_SIMD: Simd<u32, 2> = Simd([1, 5]); | ||
unsafe { | ||
let x: Simd<u8, 4> = simd_shuffle(a, b, I1_SIMD); | ||
assert_eq!(x.0, [0, 2, 4, 6]); | ||
|
||
let y: Simd<u8, 2> = simd_shuffle(a, b, I2_SIMD); | ||
assert_eq!(y.0, [1, 5]); | ||
} | ||
|
||
// Test that an indirection (via an unnamed constant) | ||
// through a const generic parameter also works. | ||
// See https://github.com/rust-lang/rust/issues/113500 for details. | ||
|
@@ -42,4 +58,11 @@ fn main() { | |
Simd<u8, 16>, | ||
>(a, b); | ||
} | ||
unsafe { | ||
__shuffle_vector16_v2::< | ||
{ Simd([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]) }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thought as above. |
||
Simd<u8, 16>, | ||
Simd<u8, 16>, | ||
>(a, b); | ||
} | ||
} |
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.
😅 I guess this is preexisting...