[PATCH] D20133: [OpenCL] Fix __builtin_astype for vec3 types.

[Yaxun Liu via cfe-commits]

yaxunl marked 7 inline comments as done.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3420
@@ -3407,35 +3419,3 @@
   llvm::Type *SrcTy = Src->getType();
-  if (isa<llvm::VectorType>(DstTy) && isa<llvm::VectorType>(SrcTy)) {
-    unsigned numElementsDst = cast<llvm::VectorType>(DstTy)->getNumElements();
-    unsigned numElementsSrc = cast<llvm::VectorType>(SrcTy)->getNumElements();
-    if ((numElementsDst == 3 && numElementsSrc == 4)
-        || (numElementsDst == 4 && numElementsSrc == 3)) {
-
-
-      // In the case of going from int4->float3, a bitcast is needed before
-      // doing a shuffle.
-      llvm::Type *srcElemTy =
-      cast<llvm::VectorType>(SrcTy)->getElementType();
-      llvm::Type *dstElemTy =
-      cast<llvm::VectorType>(DstTy)->getElementType();
-
-      if ((srcElemTy->isIntegerTy() && dstElemTy->isFloatTy())
-          || (srcElemTy->isFloatTy() && dstElemTy->isIntegerTy())) {
-        // Create a float type of the same size as the source or destination.
-        llvm::VectorType *newSrcTy = llvm::VectorType::get(dstElemTy,
-                                                                 numElementsSrc);
-
-        Src = Builder.CreateBitCast(Src, newSrcTy, "astypeCast");
-      }
-
-      llvm::Value *UnV = llvm::UndefValue::get(Src->getType());
-
-      SmallVector<llvm::Constant*, 3> Args;
-      Args.push_back(Builder.getInt32(0));
-      Args.push_back(Builder.getInt32(1));
-      Args.push_back(Builder.getInt32(2));
-
-      if (numElementsDst == 4)
-        Args.push_back(llvm::UndefValue::get(CGF.Int32Ty));
-
-      llvm::Constant *Mask = llvm::ConstantVector::get(Args);
+  unsigned numElementsSrc = isa<llvm::VectorType>(SrcTy) ?
+    cast<llvm::VectorType>(SrcTy)->getNumElements() : 0;
----------------
Anastasia wrote:
> So this code no longer applies to just vectors?
Right. The issue happens when converting vec3 type to/from non-vector types.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3427
@@ +3426,3 @@
+  // vector to get a vec4, then a bitcast if the target type is different.
+  if (numElementsSrc == 3 && numElementsDst != 3) {
+    Src = ConvertVec3AndVec4(Builder, CGF, Src, 4);
----------------
Anastasia wrote:
> Should we check numElementsDst == 4 (the same above)?
No. Special handling is needed not just for conversion between vec3/vec4 types, but also between vec3 and non-vector types.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3428
@@ +3427,3 @@
+  if (numElementsSrc == 3 && numElementsDst != 3) {
+    Src = ConvertVec3AndVec4(Builder, CGF, Src, 4);
+    Src = Builder.CreateBitCast(Src, DstTy);
----------------
Anastasia wrote:
> I am not sure why is this chosen to be this way?
> 
> If I check the OpenCL spec for type reinterpreting it doesn't seem to require shuffle vector. Also s6.2.4.2 says: 
>   "It is an error to use as_type() or as_typen() operator to reinterpret data to a type of a different number of bytes."
> 
> The only valid conversion according to the spec seems to be vec4->vec3:
> 
>   "When the operand and result type contain a different number of elements, the result shall be implementation-defined except if the operand is a 4-component vector and the result is a 3-component vector."
> 
> This change is affecting non-OpenCL code too. Is this reasonable approach for other vector implementations?
This change only affects OpenCL code since __builtin_astype is defined as a keyword for OpenCL only.

According to OpenCL spec v1.1 s6.1.5, vec3 type has the same size as vec4 type, so it is allowed to be converted to any other types which have the same size as vec4 type.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3429
@@ +3428,3 @@
+    Src = ConvertVec3AndVec4(Builder, CGF, Src, 4);
+    Src = Builder.CreateBitCast(Src, DstTy);
+    Src->setName("astype");
----------------
Anastasia wrote:
> I think we only need bitcast if type don't match?
Builder.CreateBitCast automatically handle this.

================
Comment at: test/CodeGenOpenCL/as_type.cl:7
@@ +6,3 @@
+typedef __attribute__(( ext_vector_type(3) )) int int3;
+
+//CHECK: define spir_func <3 x i8> @f1(<4 x i8> %[[x:.*]])
----------------
Anastasia wrote:
> Should this be disallowed by the frontend -? according to the spec s6.2.4.2:
> 
>   "It is an error to use as_type() or as_typen() operator to reinterpret data to a type of a different number of bytes."
According to the spec v1.1 s6.1.5, vec3 type has the same size as vec4 type, so it is allowed to be converted to other types which have the same size as vec4 type.


http://reviews.llvm.org/D20133