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

fix tests/ui/simd/issue-89193.rs and mark as passing #446

Merged
merged 3 commits into from Feb 18, 2024

Conversation

sadlerap
Copy link
Contributor

Work around an issue where usize and isize can sometimes (but not always) get canonicalized to their corresponding integer type. This causes shuffle_vector to panic, since the types of the vectors it got passed aren't the same.

Also insert a cast on the mask element, since we might get passed a signed integer of any size, not just i32. For now, we always cast to i32.

Work around an issue where usize and isize can sometimes (but not
always) get canonicalized to their corresponding integer type.  This
causes shuffle_vector to panic, since the types of the vectors it got
passed aren't the same.

Also insert a cast on the mask element, since we might get passed a
signed integer of any size, not just i32.  For now, we always cast to
i32.

Signed-off-by: Andy Sadler <andrewsadler122@gmail.com>
@sadlerap sadlerap changed the title test: mark tests/ui/simd/issue-89193.rs as passing fix tests/ui/simd/issue-89193.rs and mark as passing Feb 17, 2024
@antoyo
Copy link
Contributor

antoyo commented Feb 17, 2024

Would changing this line to the following fix the issue:

      && get_element_type ()->is_same_type_as (other_vec_type->get_element_type ())

?

I'm hoping we won't need the bitcast if we fix the type comparison in libgccjit.

@sadlerap
Copy link
Contributor Author

IMO we still need some kind of type checking within libgccjit. Element types definitely need to be "close enough" for a vector shuffle to make any kind of sense semantically, and I think relaxing type restrictions might not be the right step. I think asserting elements have the same size and alignment might be acceptable, but I don't want to take out too much here.

Some other notes:

I noticed that this function returns the signed variants of types, when it might make sense to return the unsigned variant. I tried this change, but ran into even more canonicalization issues (gcc thinks a vector of 4 u64s is distinct from a vector of 4 size_ts, which IMO is a correct assertion). I suspect this is related to the issue.

I also tried constructing the vector from default, but I got assertions that default's type wasn't a vector type? Very strange. To reproduce, add this line and run tests:

Details

diff --git a/src/intrinsic/simd.rs b/src/intrinsic/simd.rs
index 33d659f..108bdb0 100644
--- a/src/intrinsic/simd.rs
+++ b/src/intrinsic/simd.rs
@@ -708,7 +708,7 @@ pub fn generic_simd_intrinsic<'a, 'gcc, 'tcx>(
         } else {
             vector_ty(bx, underlying_ty, in_len)
         };
-        let elem_type = vector_type.dyncast_vector().expect("vector type").get_element_type();
+        let elem_type = default.get_type().dyncast_vector().expect("vector type").get_element_type();
 
         let mut values = Vec::with_capacity(in_len as usize);
         for i in 0..in_len {

Signed-off-by: Andy Sadler <andrewsadler122@gmail.com>
src/intrinsic/simd.rs Outdated Show resolved Hide resolved
@antoyo
Copy link
Contributor

antoyo commented Feb 18, 2024

Yeah, it seems that default is an aligned vector and I believe dyncast_vector() expects an unaligned (unqualified) vector.

Signed-off-by: Andy Sadler <andrewsadler122@gmail.com>
@sadlerap
Copy link
Contributor Author

As of 4ec4209, we now use default's type as the output type, and we extract the element type from there.

@antoyo antoyo merged commit 3e02db2 into rust-lang:master Feb 18, 2024
34 checks passed
@antoyo
Copy link
Contributor

antoyo commented Feb 18, 2024

Thanks for your contribution!

@sadlerap sadlerap deleted the fix-simd-gather branch February 18, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants