[OpenCL] Fix as_type3 invalid store creation

[svenvh]

Summary

With -fpreserve-vec3-type enabled, a cast was not created when
converting from a non-vec3 type to a vec3 type, even though a
conversion to vec3 was performed. This resulted in creation of
invalid store instructions.

Related to D107963, but now for the to-vec3 case.

[jaykang10]

If possible, can you add more tests for different types as previous patch please?

[svenvh]

Added more tests as requested.

[jaykang10]

Can you also add "double2_to_float3" and "float4_to_float3" tests please?

[svenvh]

In D108470#2959708, @jaykang10 wrote:

Can you also add "double2_to_float3"

Instead of double2_to_float3, I decided to add char8_to_short3. Same idea (vectorN to vector3), but reinterpreting 8 chars as 3 shorts feels like a more realistic case than reinterpreting 2 doubles as 3 floats. But I'm happy to add double2_to_float3 if you think there is a good reason to do so.

and "float4_to_float3" tests please?

That test is already there, see line 17.

[jaykang10]

In D108470#2959715, @svenvh wrote:

In D108470#2959708, @jaykang10 wrote:

Can you also add "double2_to_float3"

Instead of double2_to_float3, I decided to add char8_to_short3. Same idea (vectorN to vector3), but reinterpreting 8 chars as 3 shorts feels like a more realistic case than reinterpreting 2 doubles as 3 floats. But I'm happy to add double2_to_float3 if you think there is a good reason to do so.

I am sorry for asking more tests because I did not add enough tests previously.
I do not know well how many type conversions with vec3 the recent OpenCL spec cover but it could be good to add more the tests of the type conversions which the spec mentions. If you feel the tests you have added are enough for it, I am ok with it.

and "float4_to_float3" tests please?

That test is already there, see line 17.

Sorry, I missed it.

LGTM

[svenvh]

Instead of double2_to_float3, I decided to add char8_to_short3. Same idea (vectorN to vector3), but reinterpreting 8 chars as 3 shorts feels like a more realistic case than reinterpreting 2 doubles as 3 floats. But I'm happy to add double2_to_float3 if you think there is a good reason to do so.

I am sorry for asking more tests because I did not add enough tests previously.
I do not know well how many type conversions with vec3 the recent OpenCL spec cover but it could be good to add more the tests of the type conversions which the spec mentions. If you feel the tests you have added are enough for it, I am ok with it.

You were absolutely right about asking for more tests! I believe we now have much better coverage, let's see if @Anastasia (or any other reviewer) is also fine with the current set.

[Anastasia]

LGTM! Thanks

clang/lib/CodeGen/CGExprScalar.cpp

if (NumElementsSrc == 3 && NumElementsDst != 3) {
     Src->setName("astype");
     return Src;
   }
 
   // Going from non-vec3 to vec3 is a special case and requires a bitcast
   // to vec4 if the original type is not vec4, then a shuffle vector to
   // get a vec3.
   if (NumElementsSrc != 3 && NumElementsDst == 3) {
-    if (!CGF.CGM.getCodeGenOpts().PreserveVec3Type) {
-      auto *Vec4Ty = llvm::FixedVectorType::get(
-          cast<llvm::VectorType>(DstTy)->getElementType(), 4);
-      Src = createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(), Src,
-                                         Vec4Ty);
-    }
 
     Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
     Src->setName("astype");
     return Src;
   }
 
   return createCastsForTypeOfSameSize(Builder, CGF.CGM.getDataLayout(),
                                       Src, DstTy, "astype");

clang/test/CodeGenOpenCL/preserve_vec3.cl

 
 void from_short3(short3 a, global long *out) {
   // CHECK-LABEL: void @from_short3
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x i16> %a, <3 x i16> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
   // CHECK: %[[OUT_BC:.*]] = bitcast i64 addrspace(1)* %out to <4 x i16> addrspace(1)*
   // CHECK: store <4 x i16> %[[ASTYPE]], <4 x i16> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, long);
 }
+
+void to_char3(int a, global char3 *out) {
+  // CHECK-LABEL: void @to_char3
+  // CHECK: %[[IN_BC:.*]] = bitcast i32 %a to <4 x i8>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i8> %[[IN_BC]], <4 x i8> poison, <3 x i32> <i32 0, i32 1, i32 2>
+  // CHECK: store <3 x i8> %[[ASTYPE]], <3 x i8> addrspace(1)* %out
+  *out = __builtin_astype(a, char3);
+}