-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Port the implemention of SIMD intrinsics from Miri to const-eval #146568
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
Some changes occurred to the platform-builtins intrinsics. Make sure the cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter |
r? @davidtwco rustbot has assigned @davidtwco. Use |
|
} | ||
|
||
fn main() { | ||
const X2: i32x2 = i32x2::from_array([20, 21]); |
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.
please add checks for length 3 and 6 since portable-simd wants to support non-powers-of-2. length 3 since it's odd and length 6 because it's the smallest where the simd type's alignment is not equal to either the simd type's size or to the element's size.
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.
Should I also add these to the tests/ui/simd
file, I just copied the test cases from there?
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.
sure if you like. I mostly just figured while you're writing the code is a good time to add it to the code you're changing. I'll leave it up to the const-eval people to decide if they want it everywhere. shuffles do get a bunch of testing in portable-simd for miri (and normal compilation), but we don't explicitly test const-eval shuffles in portable-simd because they didn't exist yet.
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.
Miri and const-eval will use the same implementation so in principle it is covered.
Instead of duplicating tests we usually prefer making the existing tests run both inside and outside consts. We have some infrastructure for that in the std test suite -- is there a reason this has to be a ui test?
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.
Idts, this should be in coretests, but the tests for simd_insert
and simd_extract
was in this directory so I just put these there. I will port these to coretests
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 have moved all the tests to coretests, and enabled const-eval tests for them
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.
Actually, moving them to coretests is problematic because apparently there is no way to disable tests inside coretests (even for backends), so I undid the move, and modified the files in place to make them const-compatible
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.
What do you mean by this? There's tons of ways to disable tests inside coretests, depending on what exactly you want to do.
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 wanted to disable them based on the backend (clif and gcc has broken support, and I don't want this PR to be the one to fix them), couldn't figure it out, and then realized that the current design "works" 😅
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.
We have moved a lot of tests from ui to coretests so this feels like a step in the wrong direction. I don't know how we deal with backend issues, maybe we have cfg flags for that?
We'll have to find another reviewer for the tests anyway, a 4k diffstat is just way more than I can handle currently.
Which PR is that? Always good to cross-reference things. :) Or if the PR doesn't exist yet, what are your stdarch plans that need this? |
I want to make some stdarch functions available on const-contexts (for now, we are doing mostly constructors, but the |
I think we discussed creating a |
Also I know I mentioned it but please do not get into introducing such a new intrinsic here (a new PR, maybe!), as there are other good reasons to make this const so that we don't have to haggle over the many possible reasons (it is a truth universally acknowledged that |
Anyways, that aside, I'm basically good with this so it's mostly going to be haggling with Ralf, so r? RalfJung |
8c569d6
to
ac970b6
Compare
The Miri subtree was changed cc @rust-lang/miri |
simd_shuffle
in const_evalpub const fn from_array(a: [T; N]) -> Self { | ||
Simd(a) | ||
} | ||
|
||
pub const fn splat(value: T) -> Self | ||
where | ||
T: Copy, | ||
{ | ||
Self([value; N]) | ||
} |
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.
you need to use mem::transmute
instead of accessing the field inside a repr(simd)
struct, since last I knew the compiler doesn't properly handle the field accesses in all cases
pub const fn from_array(a: [T; N]) -> Self { | |
Simd(a) | |
} | |
pub const fn splat(value: T) -> Self | |
where | |
T: Copy, | |
{ | |
Self([value; N]) | |
} | |
pub const fn from_array(a: [T; N]) -> Self { | |
// Safety: same shape | |
unsafe { mem::transmute(a) } | |
} | |
pub const fn splat(value: T) -> Self | |
where | |
T: Copy, | |
{ | |
// Safety: same shape | |
unsafe { mem::transmute([value; N]) } | |
} |
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.
afaik constructing them is fine, projecting is the problematic part
This comment has been minimized.
This comment has been minimized.
85efca6
to
de2aa11
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
de2aa11
to
1f632cd
Compare
64804a0
to
0529a76
Compare
This comment has been minimized.
This comment has been minimized.
seems spurious, let's retry edit: huh 😕 |
☔ The latest upstream changes (presumably #147019) made this pull request unmergeable. Please resolve the merge conflicts. |
I'd prefer not expanding the scope of this PR. |
0529a76
to
1349bb8
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This is probably far better performance-wise, but is there a particular reason why these are implemented directly in miri/const-eval instead of via fallback bodies? Since I had also been investigating that path to doing this, although it's clear that some versions like |
The arguments are just random type parameters. If |
I thought that |
Even if it is perma-unstable, there are a lot more of such structs lying around. Grepping the rustc repo I could find ~300 instances of |
Fair enough! Sorry for interrupting, mostly just wanting to verify that this is the correct path for the implementation. While it doesn't seem unreasonable to just implement the fallback traits in those instances, you're right that it'd be a load of extra work when we could just rely on |
LGTM, thanks! |
Ported the implementation of most SIMD intrinsics from Miri to rustc_const_eval. Remaining are