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 SIMD Feature #5184

Merged
merged 5 commits into from Dec 8, 2023
Merged

Remove SIMD Feature #5184

merged 5 commits into from Dec 8, 2023

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Dec 7, 2023

Which issue does this PR close?

Closes #46
Closes #54
Closes #2856
Closes #5185

Rationale for this change

#5100 removed the last major use of SIMD, so we can now remove this feature. I debated deprecating things first, but this got complicated as you can't really deprecate a trait, so I figured I would at least propose just ripping the band-aid off in one.

What changes are included in this PR?

Are there any user-facing changes?

@tustvold tustvold added the api-change Changes to the arrow API label Dec 7, 2023
@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 7, 2023
@@ -610,7 +552,6 @@ pub fn gt_eq_utf8_scalar<OffsetSize: OffsetSizeTrait>(
/// Perform `left == right` operation on an array and a numeric scalar
/// value. Supports PrimitiveArrays, and DictionaryArrays that have primitive values.
///
/// If `simd` feature flag is not enabled:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hasn't actually been true since #4701

/// Evaluate `op(left, right)` for [`PrimitiveArray`]s using a specified
/// comparison function.
#[deprecated(note = "Use BooleanArray::from_binary")]
pub fn no_simd_compare_op<T, F>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to just remove these as the no_simd was confusing in the abscence of an explicit SIMD version, and they were deprecated already

/// Note that each slice should be 64 bytes and it is the callers responsibility to ensure
/// that this is the case. If passed slices larger than 64 bytes the operation will only
/// be performed on the first 64 bytes. Slices less than 64 bytes will panic.
#[cfg(feature = "simd")]
Copy link
Contributor Author

@tustvold tustvold Dec 7, 2023

Choose a reason for hiding this comment

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

Interestingly arrow-buffer doesn't have a SIMD feature, so this has been silently not present by accident for a while. Likely since #2693

@jhorstmann
Copy link
Contributor

Ok, that then also fixes #5185 which I just created :)

@jhorstmann
Copy link
Contributor

Changes look good to me 👍

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks good to me. We've not used this simd feature but rely on auto-vectorization. This makes the code more clean.

Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

Great job. Thanks @tustvold
Appreciate #5100 of @jhorstmann

@tustvold tustvold merged commit d41e90e into apache:master Dec 8, 2023
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate
Projects
None yet
4 participants