Skip to content
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

implement SIMD gather/scatter via vector getelementptr #95961

Merged
merged 3 commits into from
Apr 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,27 @@ unsupported {} from `{}` with element `{}` of size `{}` to `{}`"#,
simd_neg: Int => neg, Float => fneg;
}

if name == sym::simd_arith_offset {
// This also checks that the first operand is a ptr type.
let pointee = in_elem.builtin_deref(true).unwrap_or_else(|| {
span_bug!(span, "must be called with a vector of pointer types as first argument")
});
let layout = bx.layout_of(pointee.ty);
let ptrs = args[0].immediate();
// The second argument must be a ptr-sized integer.
// (We don't care about the signedness, this is wrapping anyway.)
let (_offsets_len, offsets_elem) = arg_tys[1].simd_size_and_type(bx.tcx());
if !matches!(offsets_elem.kind(), ty::Int(ty::IntTy::Isize) | ty::Uint(ty::UintTy::Usize)) {
span_bug!(
span,
"must be called with a vector of pointer-sized integers as second argument"
);
}
let offsets = args[1].immediate();

return Ok(bx.gep(bx.backend_type(layout), ptrs, &[offsets]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does gep type check this? Or can calling the intrinsic improperly result in an LLVM codegen error?

Copy link
Member

@workingjubilee workingjubilee Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, so we should probably add a codegen test rather than finding out the hard way. gep isn't that complex but it's known to ambush unwary compiler engineers when they turn their back on it. I can help draft such a codegen test if necessary.

Copy link
Member Author

@RalfJung RalfJung Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does about as much checking as the arith_offset intrinsic, I think. The one extra check there is that the offsets must be ptr-sized integers, which arith_offset enforces via the type signature (but I am not sure if anything checks that type signature). I can try to add that here.

What kind of codegen test are you looking for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check for the integer type in the second operand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I mostly just want to see with this that we generate a vector of pointers to the given type and then gep it like we Damn Well Should and don't suddenly somehow revert to scalar operations or something like that. A smoke test that the suite of intrinsics used to do a gather or scatter compiles correctly, basically, and that the types don't go suddenly weird on us.

This does about as much checking as the arith_offset intrinsic, I think. The one extra check there is that the offsets must be ptr-sized integers, which arith_offset enforces via the type signature (but I am not sure if anything checks that type signature). I can try to add that here.

For the #[repr(simd)] types, while we do type-checking in rustc's front and "middle" phases to guarantee the input vector types are valid machine vector types, you can assume that it probably has bypassed any sensible checking like "is this even actually a pointer?" and is relying heavily on correct usage. Thus we would like to error during monomorphization on anything fishy.

Copy link
Member

@workingjubilee workingjubilee Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is to say:

It is important to error in mono (as you do here) because I do not believe we have anything before this step that would even enforce that the first arg to simd_arith_offset would be a vector of pointers.

Copy link
Member Author

@RalfJung RalfJung Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I mostly just want to see with this that we generate a vector of pointers to the given type

Okay... I'll try my best but I barely ever work with codegen tests so I am not even sure of the syntax to use.^^ Is there another portable-simd codegen test I could model this off of?
Should it go through portable-simd APIs or call the intrinsics directly?

A smoke test that the suite of intrinsics used to do a gather or scatter compiles correctly, basically, and that the types don't go suddenly weird on us.

We have the doctests as smoke tests as well. ;)

}

if name == sym::simd_saturating_add || name == sym::simd_saturating_sub {
let lhs = args[0].immediate();
let rhs = args[1].immediate();
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1245,6 +1245,7 @@ symbols! {
simd,
simd_add,
simd_and,
simd_arith_offset,
simd_as,
simd_bitmask,
simd_cast,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_typeck/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@ pub fn check_platform_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>)
| sym::simd_fpow
| sym::simd_saturating_add
| sym::simd_saturating_sub => (1, vec![param(0), param(0)], param(0)),
sym::simd_arith_offset => (2, vec![param(0), param(1)], param(0)),
sym::simd_neg
| sym::simd_fsqrt
| sym::simd_fsin
Expand Down
4 changes: 4 additions & 0 deletions library/portable-simd/crates/core_simd/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ extern "platform-intrinsic" {
/// xor
pub(crate) fn simd_xor<T>(x: T, y: T) -> T;

/// getelementptr (without inbounds)
#[cfg(not(bootstrap))]
pub(crate) fn simd_arith_offset<T, U>(ptrs: T, offsets: U) -> T;

/// fptoui/fptosi/uitofp/sitofp
/// casting floats to integers is truncating, so it is safe to convert values like e.g. 1.5
/// but the truncated value must fit in the target type or the result is poison.
Expand Down
11 changes: 11 additions & 0 deletions library/portable-simd/crates/core_simd/src/vector/ptr.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Private implementation details of public gather/scatter APIs.
#[cfg(not(bootstrap))]
use crate::simd::intrinsics;
use crate::simd::{LaneCount, Simd, SupportedLaneCount};
#[cfg(bootstrap)]
use core::mem;

/// A vector of *const T.
Expand All @@ -21,12 +24,16 @@ where
#[inline]
#[must_use]
pub fn wrapping_add(self, addend: Simd<usize, LANES>) -> Self {
#[cfg(bootstrap)]
// Safety: converting pointers to usize and vice-versa is safe
// (even if using that pointer is not)
unsafe {
let x: Simd<usize, LANES> = mem::transmute_copy(&self);
mem::transmute_copy(&{ x + (addend * Simd::splat(mem::size_of::<T>())) })
}
#[cfg(not(bootstrap))]
// Safety: this intrinsic doesn't have a precondition
unsafe { intrinsics::simd_arith_offset(self, addend) }
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -49,11 +56,15 @@ where
#[inline]
#[must_use]
pub fn wrapping_add(self, addend: Simd<usize, LANES>) -> Self {
#[cfg(bootstrap)]
// Safety: converting pointers to usize and vice-versa is safe
// (even if using that pointer is not)
unsafe {
let x: Simd<usize, LANES> = mem::transmute_copy(&self);
mem::transmute_copy(&{ x + (addend * Simd::splat(mem::size_of::<T>())) })
}
#[cfg(not(bootstrap))]
// Safety: this intrinsic doesn't have a precondition
unsafe { intrinsics::simd_arith_offset(self, addend) }
}
}
26 changes: 26 additions & 0 deletions src/test/codegen/simd_arith_offset.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// compile-flags: -C no-prepopulate-passes
// only-64bit (because the LLVM type of i64 for usize shows up)
//

#![crate_type = "lib"]
#![feature(repr_simd, platform_intrinsics)]

extern "platform-intrinsic" {
pub(crate) fn simd_arith_offset<T, U>(ptrs: T, offsets: U) -> T;
}

/// A vector of *const T.
#[derive(Debug, Copy, Clone)]
#[repr(simd)]
pub struct SimdConstPtr<T, const LANES: usize>([*const T; LANES]);

#[derive(Debug, Copy, Clone)]
#[repr(simd)]
pub struct Simd<T, const LANES: usize>([T; LANES]);

// CHECK-LABEL: smoke
#[no_mangle]
pub fn smoke(ptrs: SimdConstPtr<u8, 8>, offsets: Simd<usize, 8>) -> SimdConstPtr<u8, 8> {
// CHECK: getelementptr i8, <8 x i8*> %_3, <8 x i64> %_4
unsafe { simd_arith_offset(ptrs, offsets) }
}