-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Stop projecting into SIMD types in ui/simd/
tests
#138036
base: master
Are you sure you want to change the base?
Conversation
Part of MCP838
Are u planning on making field access on repr(simd) in HIR a hard error as the next step or what? |
@compiler-errors Well, I need to get a stdarch PR in before I can truly ban anything. Locally I've got it banned in the MIR validator (as the fastest way to get an ugly error about it) but a HIR one would certainly be better. EDIT: stdarch PR rust-lang/stdarch#1740 |
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.
There's nothing wrong w these changes necessarily, but I do feel several tests took a slightly different migration strategy, and they could be made a bit more consistent for better readability.
let a = $a; | ||
let b = $b; | ||
assert_eq!(a.to_array(), b.to_array()); |
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.
is this necessary? can we not just use $a.to_array()
? Also maybe rename this to assert_eq_simd!
like the tests below?
fn to_array(self) -> [i32; N] { unsafe { std::intrinsics::transmute_unchecked(self) } } | ||
} | ||
|
||
macro_rules! all_eq { |
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.
Same as above.
for (u, (uu32, ui64)) in u | ||
.to_array() | ||
.iter() | ||
.zip(uu32.to_array().iter().zip(ui64.to_array().iter())) |
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.
nitpick but u.to_array().into_iter().zip(uu32.to_array()).zip(ui64.to_array())
is a bit simplier 🤔
let [b, a] = [$b, $a]; | ||
let [b, a] = [b.to_array(), a.to_array()]; |
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.
this is pretty awkward lol, is this really necessary for the 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.
let a = $a.to_array();
let b = $b.to_array();
Or else just replace this with the same assert_eq_simd
macro as everything else.
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.
Yeah, this one was weird. As the comment says, it was already doing weird things with type inference. I'll see if I can simplify it...
@@ -27,7 +31,7 @@ fn main() { | |||
|
|||
let r_strided = simd_gather(default, pointers, mask); | |||
|
|||
assert_eq!(r_strided, s_strided); | |||
assert_eq!(r_strided.to_array(), s_strided.to_array()); |
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.
macro-wise, should we replace this with assert_eq_simd!
rather than calling to_array
everywhere?
|
||
macro_rules! all_eq { | ||
($a: expr, $b: expr) => {{ | ||
let a = $a; |
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.
Ping
assert!(const_ptrs.0 == [ptr as *const u8, core::ptr::null()]); | ||
assert!(exposed_addr.0 == [ptr as usize, 0]); | ||
assert!(with_exposed_provenance.0 == ptrs.0); | ||
assert!(const_ptrs.to_array() == [ptr as *const u8, core::ptr::null()]); |
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.
make this more unified w the rest of the tests
Oh, yeah, MIR validation is probably fine too. It can be an ICE; it doesn't need to be a good diagnostic. |
Part of MCP838
This intentionally leaves projections in two places, as a double-check that what stdarch is still using (for now) continues to work
But the things that aren't testing that change to not using
.0
on therepr(simd)
type.