[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

 // RUN: %clang_cc1 %s -emit-llvm -o - -triple spir-unknown-unknown -fpreserve-vec3-type | FileCheck %s
 
 typedef char char3 __attribute__((ext_vector_type(3)));
+typedef char char8 __attribute__((ext_vector_type(8)));
 typedef short short3 __attribute__((ext_vector_type(3)));
 typedef double double2 __attribute__((ext_vector_type(2)));
 typedef float float3 __attribute__((ext_vector_type(3)));
 typedef float float4 __attribute__((ext_vector_type(4)));
 
 void kernel foo(global float3 *a, global float3 *b) {
   // CHECK-LABEL: spir_kernel void @foo
   // CHECK: %[[LOAD_A:.*]] = load <3 x float>, <3 x float> addrspace(1)* %a
void kernel float3_to_double2(global float3 *a, global double2 *b) {
   // CHECK-LABEL: spir_kernel void @float3_to_double2
   // CHECK: %[[LOAD_A:.*]] = load <3 x float>, <3 x float> addrspace(1)* %a, align 16
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x float> %[[LOAD_A]], <3 x float> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
   // CHECK: %[[OUT_BC:.*]] = bitcast <2 x double> addrspace(1)* %b to <4 x float> addrspace(1)*
   // CHECK: store <4 x float> %[[ASTYPE]], <4 x float> addrspace(1)* %[[OUT_BC]], align 16
   *b = __builtin_astype(*a, double2);
 }
 
+void kernel char8_to_short3(global short3 *a, global char8 *b) {
+  // CHECK-LABEL: spir_kernel void @char8_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast <8 x i8> addrspace(1)* %b to <4 x i16> addrspace(1)*
+  // CHECK: %[[LOAD_B:.*]] = load <4 x i16>, <4 x i16> addrspace(1)* %[[IN_BC]]
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[LOAD_B]], <4 x i16> poison, <3 x i32> <i32 0, i32 1, i32 2>
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %a, align 8
+  *a = __builtin_astype(*b, short3);
+}
+
 void from_char3(char3 a, global int *out) {
   // CHECK-LABEL: void @from_char3
   // CHECK: %[[ASTYPE:.*]] = shufflevector <3 x i8> %a, <3 x i8> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
   // CHECK: %[[OUT_BC:.*]] = bitcast i32 addrspace(1)* %out to <4 x i8> addrspace(1)*
   // CHECK: store <4 x i8> %[[ASTYPE]], <4 x i8> addrspace(1)* %[[OUT_BC]]
   *out = __builtin_astype(a, int);
 }
 
 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 scalar_to_char3(int a, global char3 *out) {
+  // CHECK-LABEL: void @scalar_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);
+}
+
+void scalar_to_short3(long a, global short3 *out) {
+  // CHECK-LABEL: void @scalar_to_short3
+  // CHECK: %[[IN_BC:.*]] = bitcast i64 %a to <4 x i16>
+  // CHECK: %[[ASTYPE:.*]] = shufflevector <4 x i16> %[[IN_BC]], <4 x i16> poison, <3 x i32> <i32 0, i32 1, i32 2>
+  // CHECK: store <3 x i16> %[[ASTYPE]], <3 x i16> addrspace(1)* %out
+  *out = __builtin_astype(a, short3);
+}