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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3403
@@ +3402,3 @@
+static Value *ConvertVec3AndVec4(CGBuilderTy &Builder, CodeGenFunction &CGF,
+  Value *Src, unsigned numElementsDst) {
+  llvm::Value *UnV = llvm::UndefValue::get(Src->getType());
----------------
numElementsDst -> NumElementsDst

Also I would expect formatter to align to the parameter list area...

================
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;
----------------
So this code no longer applies to just vectors?

================
Comment at: lib/CodeGen/CGExprScalar.cpp:3422
@@ +3421,3 @@
+    cast<llvm::VectorType>(SrcTy)->getNumElements() : 0;
+  unsigned numElementsDst = isa<llvm::VectorType>(DstTy) ?
+    cast<llvm::VectorType>(DstTy)->getNumElements() : 0;
----------------
numElementsSrc -> NumElementsSrc

The same below!

================
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);
----------------
Should we check numElementsDst == 4 (the same above)?

================
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);
----------------
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?

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

================
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:.*]])
----------------
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."


http://reviews.llvm.org/D20133