rust-lang / rustc_codegen_gcc

libgccjit AOT codegen for rustc
Apache License 2.0
894 stars 61 forks source link

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

Closed sadlerap closed 4 months ago

sadlerap commented 4 months ago

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.

antoyo commented 4 months ago

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 commented 4 months ago

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 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 { ```

antoyo commented 4 months ago

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

sadlerap commented 4 months ago

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

antoyo commented 4 months ago

Thanks for your contribution!