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

Remove generic_const_exprs #368

Merged
merged 4 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
12 changes: 4 additions & 8 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ jobs:
run: cross test --verbose --target=${{ matrix.target }} --release

features:
name: "Check cargo features (${{ matrix.simd }} × ${{ matrix.features }})"
name: "Test cargo features (${{ matrix.simd }} × ${{ matrix.features }})"
runs-on: ubuntu-latest
strategy:
fail-fast: false
Expand All @@ -249,12 +249,8 @@ jobs:
features:
- ""
- "--features std"
- "--features generic_const_exprs"
- "--features std --features generic_const_exprs"
- "--features all_lane_counts"
- "--features all_lane_counts --features std"
- "--features all_lane_counts --features generic_const_exprs"
- "--features all_lane_counts --features std --features generic_const_exprs"
- "--all-features"

steps:
- uses: actions/checkout@v2
Expand All @@ -266,9 +262,9 @@ jobs:
run: echo "CPU_FEATURE=$(lscpu | grep -o avx512[a-z]* | sed s/avx/+avx/ | tr '\n' ',' )" >> $GITHUB_ENV
- name: Check build
if: ${{ matrix.simd == '' }}
run: RUSTFLAGS="-Dwarnings" cargo check --all-targets --no-default-features ${{ matrix.features }}
run: RUSTFLAGS="-Dwarnings" cargo test --all-targets --no-default-features ${{ matrix.features }}
- name: Check AVX
if: ${{ matrix.simd == 'avx512' && contains(env.CPU_FEATURE, 'avx512') }}
run: |
echo "Found AVX features: $CPU_FEATURE"
RUSTFLAGS="-Dwarnings -Ctarget-feature=$CPU_FEATURE" cargo check --all-targets --no-default-features ${{ matrix.features }}
RUSTFLAGS="-Dwarnings -Ctarget-feature=$CPU_FEATURE" cargo test --all-targets --no-default-features ${{ matrix.features }}
1 change: 0 additions & 1 deletion crates/core_simd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ license = "MIT OR Apache-2.0"
default = ["as_crate"]
as_crate = []
std = []
generic_const_exprs = []
all_lane_counts = []

[target.'cfg(target_arch = "wasm32")'.dev-dependencies]
Expand Down
2 changes: 0 additions & 2 deletions crates/core_simd/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
strict_provenance,
ptr_metadata
)]
#![cfg_attr(feature = "generic_const_exprs", feature(generic_const_exprs))]
#![cfg_attr(feature = "generic_const_exprs", allow(incomplete_features))]
#![warn(missing_docs, clippy::missing_inline_in_public_items)] // basically all items, really
#![deny(unsafe_op_in_unsafe_fn, clippy::undocumented_unsafe_blocks)]
#![allow(internal_features)]
Expand Down
5 changes: 1 addition & 4 deletions crates/core_simd/src/masks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@
mod mask_impl;

mod to_bitmask;
pub use to_bitmask::ToBitMask;

#[cfg(feature = "generic_const_exprs")]
pub use to_bitmask::{bitmask_len, ToBitMaskArray};
pub use to_bitmask::{ToBitMask, ToBitMaskArray};

use crate::simd::{intrinsics, LaneCount, Simd, SimdElement, SimdPartialEq, SupportedLaneCount};
use core::cmp::Ordering;
Expand Down
2 changes: 0 additions & 2 deletions crates/core_simd/src/masks/bitmask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ where
unsafe { Self(intrinsics::simd_bitmask(value), PhantomData) }
}

#[cfg(feature = "generic_const_exprs")]
#[inline]
#[must_use = "method returns a new array and does not mutate the original value"]
pub fn to_bitmask_array<const N: usize>(self) -> [u8; N] {
Expand All @@ -129,7 +128,6 @@ where
unsafe { core::mem::transmute_copy(&self.0) }
}

#[cfg(feature = "generic_const_exprs")]
#[inline]
#[must_use = "method returns a new mask and does not mutate the original value"]
pub fn from_bitmask_array<const N: usize>(bitmask: [u8; N]) -> Self {
Expand Down
25 changes: 7 additions & 18 deletions crates/core_simd/src/masks/full_masks.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
//! Masks that take up full SIMD vector registers.

use super::MaskElement;
use super::{to_bitmask::ToBitMaskArray, MaskElement};
use crate::simd::intrinsics;
use crate::simd::{LaneCount, Simd, SupportedLaneCount, ToBitMask};

#[cfg(feature = "generic_const_exprs")]
use crate::simd::ToBitMaskArray;

#[repr(transparent)]
pub struct Mask<T, const LANES: usize>(Simd<T, LANES>)
where
Expand Down Expand Up @@ -145,23 +142,19 @@ where
unsafe { Mask(intrinsics::simd_cast(self.0)) }
}

#[cfg(feature = "generic_const_exprs")]
#[inline]
#[must_use = "method returns a new array and does not mutate the original value"]
pub fn to_bitmask_array<const N: usize>(self) -> [u8; N]
where
super::Mask<T, LANES>: ToBitMaskArray,
[(); <super::Mask<T, LANES> as ToBitMaskArray>::BYTES]: Sized,
{
assert_eq!(<super::Mask<T, LANES> as ToBitMaskArray>::BYTES, N);

// Safety: N is the correct bitmask size
// Safety: Bytes is the right size array
unsafe {
// Compute the bitmask
let bitmask: [u8; <super::Mask<T, LANES> as ToBitMaskArray>::BYTES] =
let bitmask: <super::Mask<T, LANES> as ToBitMaskArray>::BitMaskArray =
intrinsics::simd_bitmask(self.0);

// Transmute to the return type, previously asserted to be the same size
// Transmute to the return type
let mut bitmask: [u8; N] = core::mem::transmute_copy(&bitmask);

// LLVM assumes bit order should match endianness
Expand All @@ -175,17 +168,13 @@ where
}
}

#[cfg(feature = "generic_const_exprs")]
#[inline]
#[must_use = "method returns a new mask and does not mutate the original value"]
pub fn from_bitmask_array<const N: usize>(mut bitmask: [u8; N]) -> Self
where
super::Mask<T, LANES>: ToBitMaskArray,
[(); <super::Mask<T, LANES> as ToBitMaskArray>::BYTES]: Sized,
{
assert_eq!(<super::Mask<T, LANES> as ToBitMaskArray>::BYTES, N);

// Safety: N is the correct bitmask size
// Safety: Bytes is the right size array
unsafe {
// LLVM assumes bit order should match endianness
if cfg!(target_endian = "big") {
Expand All @@ -194,8 +183,8 @@ where
}
}

// Transmute to the bitmask type, previously asserted to be the same size
let bitmask: [u8; <super::Mask<T, LANES> as ToBitMaskArray>::BYTES] =
// Transmute to the bitmask
let bitmask: <super::Mask<T, LANES> as ToBitMaskArray>::BitMaskArray =
core::mem::transmute_copy(&bitmask);

// Compute the regular mask
Expand Down
64 changes: 34 additions & 30 deletions crates/core_simd/src/masks/to_bitmask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,18 @@ pub trait ToBitMask: Sealed {
/// Converts masks to and from byte array bitmasks.
///
/// Each bit of the bitmask corresponds to a mask lane, starting with the LSB of the first byte.
#[cfg(feature = "generic_const_exprs")]
pub trait ToBitMaskArray: Sealed {
/// The length of the bitmask array.
const BYTES: usize;
/// The bitmask array.
type BitMaskArray;
Copy link
Member

Choose a reason for hiding this comment

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

this should have some useful bounds, such as Copy + Unpin + Send + Sync + 'static + AsRef<[u8]> + AsMut<[u8]> + BorrowMut<[u8]> and maybe more

Copy link
Member Author

Choose a reason for hiding this comment

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

Would adding more bounds in the future be backwards compatible?

Copy link
Member

@workingjubilee workingjubilee Oct 2, 2023

Choose a reason for hiding this comment

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

No, bounds can only be relaxed backwards-compatibly, and only sometimes. They cannot be added, so forward-compatibility requires maximum bounds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that still true when the trait is sealed?

Copy link
Member

Choose a reason for hiding this comment

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

...that does change things a bit, since the major compat hazard is people referencing the type generically.


/// Converts a mask to a bitmask.
fn to_bitmask_array(self) -> [u8; Self::BYTES];
fn to_bitmask_array(self) -> Self::BitMaskArray;

/// Converts a bitmask to a mask.
fn from_bitmask_array(bitmask: [u8; Self::BYTES]) -> Self;
fn from_bitmask_array(bitmask: Self::BitMaskArray) -> Self;
}

macro_rules! impl_integer_intrinsic {
macro_rules! impl_integer {
{ $(impl ToBitMask<BitMask=$int:ty> for Mask<_, $lanes:literal>)* } => {
$(
impl<T: MaskElement> ToBitMask for Mask<T, $lanes> {
Expand All @@ -62,7 +61,27 @@ macro_rules! impl_integer_intrinsic {
}
}

impl_integer_intrinsic! {
macro_rules! impl_array {
{ $(impl ToBitMaskArray<Bytes=$int:literal> for Mask<_, $lanes:literal>)* } => {
$(
impl<T: MaskElement> ToBitMaskArray for Mask<T, $lanes> {
type BitMaskArray = [u8; $int];

#[inline]
fn to_bitmask_array(self) -> Self::BitMaskArray {
self.0.to_bitmask_array()
}

#[inline]
fn from_bitmask_array(bitmask: Self::BitMaskArray) -> Self {
Self(mask_impl::Mask::from_bitmask_array(bitmask))
}
}
)*
}
}

impl_integer! {
impl ToBitMask<BitMask=u8> for Mask<_, 1>
impl ToBitMask<BitMask=u8> for Mask<_, 2>
impl ToBitMask<BitMask=u8> for Mask<_, 4>
Expand All @@ -72,27 +91,12 @@ impl_integer_intrinsic! {
impl ToBitMask<BitMask=u64> for Mask<_, 64>
}

/// Returns the minimum number of bytes in a bitmask with `lanes` lanes.
#[cfg(feature = "generic_const_exprs")]
#[allow(clippy::missing_inline_in_public_items)]
pub const fn bitmask_len(lanes: usize) -> usize {
(lanes + 7) / 8
}

#[cfg(feature = "generic_const_exprs")]
impl<T: MaskElement, const LANES: usize> ToBitMaskArray for Mask<T, LANES>
where
LaneCount<LANES>: SupportedLaneCount,
{
const BYTES: usize = bitmask_len(LANES);

#[inline]
fn to_bitmask_array(self) -> [u8; Self::BYTES] {
self.0.to_bitmask_array()
}

#[inline]
fn from_bitmask_array(bitmask: [u8; Self::BYTES]) -> Self {
Mask(mask_impl::Mask::from_bitmask_array(bitmask))
}
impl_array! {
impl ToBitMaskArray<Bytes=1> for Mask<_, 1>
impl ToBitMaskArray<Bytes=1> for Mask<_, 2>
impl ToBitMaskArray<Bytes=1> for Mask<_, 4>
impl ToBitMaskArray<Bytes=1> for Mask<_, 8>
impl ToBitMaskArray<Bytes=2> for Mask<_, 16>
impl ToBitMaskArray<Bytes=4> for Mask<_, 32>
impl ToBitMaskArray<Bytes=8> for Mask<_, 64>
}
5 changes: 2 additions & 3 deletions crates/core_simd/src/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ mod swizzle;

pub(crate) mod intrinsics;

#[cfg(feature = "generic_const_exprs")]
mod to_bytes;

mod alias;
mod cast;
mod elements;
Expand All @@ -18,6 +15,7 @@ mod ops;
mod ord;
mod select;
mod swizzle_dyn;
mod to_bytes;
mod vector;
mod vendor;

Expand All @@ -37,5 +35,6 @@ pub mod simd {
pub use crate::core_simd::ord::*;
pub use crate::core_simd::swizzle::*;
pub use crate::core_simd::swizzle_dyn::*;
pub use crate::core_simd::to_bytes::ToBytes;
pub use crate::core_simd::vector::*;
}
Loading
Loading