-
Notifications
You must be signed in to change notification settings - Fork 296
Implement arithmetic operation traits for x86 SIMD types #1898
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
base: master
Are you sure you want to change the base?
Conversation
r? @folkertdev rustbot has assigned @folkertdev. Use |
CI is stuck on #1897 🤦🏽 |
yeah that will likely take a couple of days at least |
macro_rules! impl_arith_op { | ||
(__internal $op:ident, $intrinsic:ident $_:ident) => { | ||
#[inline] | ||
fn $op(self, rhs: Self) -> Self { |
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.
These are missing a #[target_feature(enable = "...")]
matching the _mm*
caller, so the MIR inliner will not inline them and LLVM may not inline them either.
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.
simd_*
intrinsics call the platform-independent LLVM intrinsics/instructions. The conversion to ASM checks the available feature flags, and uses them as available. See rust-lang/libs-team#628 (comment)
Do you want to decide on those unresolved questions here, or later, or bounce it back to T-libs?
|
Also @Amanieu merging this PR insta-stabilizes these instances, so just checking: that is intentional, right? I feel like in that case we should at least implement the same operations for all stable simd types in the same rust release, i.e. at least make sure the aarch64 ones make it into the same release as the x86 ones. |
Any instantly-stable API needs a libs-api FCP, even if an ACP was accepted. I can nominate this for libs-api discussion around Not/Rem, but I believe at least in the last discussion we decided against having Personally, I'm having second thoughts about having these trait implementations at all: I feel that in the long term what you really want to do is use the portable SIMD API most of the time and only occasionally dip into the target-specific intrinsics where needed. I also feel that these trait implementations can be a performance footgun because they may give you the impression that SIMD support is available for an operation (e.g. division) when it isn't actually supported by hardware. |
ACP: rust-lang/libs-team#628
Tracking Issue: rust-lang/rust#145066
Unresolved questions: Should we have
Not
andRem
implementations? At leastNot
seems quite useful.I will send the PRs of other archs after this is merged, as they also use the common macro