Convert some SSE2 intrinsics to const generics #1021
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| // | ||
| // Of course, that's... awful. So we try to use macros to do it for us. | ||
| let imm8 = (imm8 & 0xFF) as u8; | ||
| pub unsafe fn _mm_shuffle_epi32<const imm8: i32>(a: __m128i) -> __m128i { |
There was a problem hiding this comment.
Yep, same thing happened in a previous PR. We're kinda allowed to file it as "bugfix" though, because the api shouldn't accept a value outside 0..=255
There was a problem hiding this comment.
yes, similarly to _mm_shuffle_ps discussed here #1018 (comment) which was acceptable as a bug fix if I understand correctly ?
There was a problem hiding this comment.
Well, we could make it still accept any value and do &0xFF if we wanted to preserve the bug.
There was a problem hiding this comment.
sure, whichever you all prefer. my understanding is that a crater run will ultimately help decide whether the bug should be preserved if need be ?
There was a problem hiding this comment.
Ah I see. If this is only going to break things where imm is not a constant, then I'm okay with that and agree it is a bug fix. I am only concerned about function params being changed.
I guess i would also be concerned with how this renders in rustdoc, but whatever the case, that seems fixable.
There was a problem hiding this comment.
Some initial discussion about this feature can be also be seen in this other zulip link, leading to the rustc PR linked above. Some more specific info about stdarch and the migration effort is in #1022.
There was a problem hiding this comment.
Note that #[rustc_args_required_const] already required the value to be a constant.
There was a problem hiding this comment.
Does that mean this change cannot break anything?
There was a problem hiding this comment.
There is one specific situation where it can break: you can't use an expression derived from generic arguments in a const item (e.g. const or a const generic parameter). But #[rustc_args_required_const] accepted this.
Example
fn foo<const X: i32>(a: __m128) {
// This was previously accepted but will now fail to compile.
stdarch::_mm_shuffle_ps(a, a, X + 1);
// This still works.
stdarch::_mm_shuffle_ps(a, a, X);
}| } | ||
| constify_imm8!(imm8, call) | ||
| pub unsafe fn _mm_srai_epi16<const imm8: i32>(a: __m128i) -> __m128i { | ||
| transmute(psraiw(a.as_i16x8(), imm8)) |
There was a problem hiding this comment.
This should have a static_assert! to ensure the immediate is between 0 and 255.
There was a problem hiding this comment.
Looks like static_assert!(imm8: i32 where imm8 >= 0 && imm8 <= 255); is going to be used quite a bit, we could have a static_assert_imm8! or in general static asserts analogues to the various constify_imm* ?
There was a problem hiding this comment.
That sounds like a good idea. Also we should use a single shared Validate struct for those to reduce the MIR size (rather than instantiating a new one in each function).
The comment "immediate value: -16:15" makes it look it should have been named `constify_imm_s5` but its body is a duplicate of another `constify_imm5`
0bd1315 to
3d35abb
Compare
Since this one will be used a lot, a single macro and struct can be used to avoid duplicating the imm8 check in every function, and instantiating the same MIR struct multiple times.
… `imm8` is in range
3d35abb to
386e978
Compare
This PR switches:
_mm_shuffle_epi32_mm_shufflehi_epi16_mm_shufflelo_epi16_mm_srai_epi16_mm_srai_epi32to const generics using
#[rustc_legacy_const_generics]