[OpenCL] Fix __builtin_astype for vec3 types.

[yaxunl]

Summary

__builtin_astype does not generate correct LLVM IR for vec3 types. This patch inserts bitcasts to/from vec4 when necessary in addition to generating vector shuffle. A codegen test is added.

[bader]

LGTM.

test/CodeGenOpenCL/as_type.cl

6 ↗

(On Diff #56809)

Could you also one test case that requires bitcast from vector type to 4-element vector type, please?
E.g. short2 <-> char3 or char16 <-> int3.

[yaxunl]

Add a test for casting char16 to i3 as Alexey suggested.

[yaxunl]

Update test.

[Anastasia]

lib/CodeGen/CGExprScalar.cpp

3403 ↗

(On Diff #57393)

numElementsDst -> NumElementsDst

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

3420 ↗

(On Diff #57393)

So this code no longer applies to just vectors?

3422 ↗

(On Diff #57393)

numElementsSrc -> NumElementsSrc

The same below!

3427 ↗

(On Diff #57393)

Should we check numElementsDst == 4 (the same above)?

3428 ↗

(On Diff #57393)

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?

3429 ↗

(On Diff #57393)

I think we only need bitcast if type don't match?

test/CodeGenOpenCL/as_type.cl

7 ↗

(On Diff #57393)

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."

[yaxunl]

lib/CodeGen/CGExprScalar.cpp

3420 ↗

(On Diff #57393)

Right. The issue happens when converting vec3 type to/from non-vector types.

3427 ↗

(On Diff #57393)

No. Special handling is needed not just for conversion between vec3/vec4 types, but also between vec3 and non-vector types.

3428 ↗

(On Diff #57393)

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.

3429 ↗

(On Diff #57393)

Builder.CreateBitCast automatically handle this.

test/CodeGenOpenCL/as_type.cl

7 ↗

(On Diff #57393)

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.

[yaxunl]

Fix variable capitalization and indentation.

[Anastasia]

lib/CodeGen/CGExprScalar.cpp

3394 ↗

(On Diff #58991)

should this be 3 unstead of undef?

3428 ↗

(On Diff #58991)

I see. Not related to your change, but I was just wondering if it would be better to change this to a Clang builtin with a custom check at some point. It would be easier to understand and we can avoid all this parsing/AST handling complications.

test/CodeGenOpenCL/as_type.cl

8 ↗

(On Diff #58991)

So what happens if the number of bytes don't match?

46 ↗

(On Diff #58991)

Would it make sense to check that shufflevector is not generated?

53 ↗

(On Diff #58991)

Could we add CHECK-NOT bitcast here?

[yaxunl]

Add a sema test for mismatched type size. Fix codegen test.

lib/CodeGen/CGExprScalar.cpp

3394 ↗

(On Diff #58991)

3 or undef are both OK here since the 4th component is undefined. undef may be better since if optimizer checks mask first, it knows that the 4th component is undefined without further checking the second operand.

test/CodeGenOpenCL/as_type.cl

8 ↗

(On Diff #58991)

There will be compilation error and diag msg. I will add a sema test.

46 ↗

(On Diff #58991)

will fix

53 ↗

(On Diff #58991)

will fix

[Anastasia]

LGTM!

[yaxunl]

Ping

Xiuli, could you please take a look? Thanks.

[pxli168]

I was on a vecation. LGTM, thanks!

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

Value *ScalarExprEmitter::VisitVAArgExpr(VAArgExpr *VE) {
 
   return Val;
 }
 
 Value *ScalarExprEmitter::VisitBlockExpr(const BlockExpr *block) {
   return CGF.EmitBlockLiteral(block);
 }
 
-Value *ScalarExprEmitter::VisitAsTypeExpr(AsTypeExpr *E) {
-  Value *Src  = CGF.EmitScalarExpr(E->getSrcExpr());
-  llvm::Type *DstTy = ConvertType(E->getType());
-
-  // Going from vec4->vec3 or vec3->vec4 is a special case and requires
-  // a shuffle vector instead of a bitcast.
-  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);
+// Convert a vec3 to vec4, or vice versa.
+static Value *ConvertVec3AndVec4(CGBuilderTy &Builder, CodeGenFunction &CGF,
+                                 Value *Src, unsigned NumElementsDst) {
+  SmallVector<llvm::Constant*, 4> Args;
+  if (NumElementsDst == 4)
+  return Builder.CreateShuffleVector(Src, UnV, Mask);
+}
+
+Value *ScalarExprEmitter::VisitAsTypeExpr(AsTypeExpr *E) {
+  Value *Src  = CGF.EmitScalarExpr(E->getSrcExpr());
+  llvm::Type *DstTy = ConvertType(E->getType());
 
-      return Builder.CreateShuffleVector(Src, UnV, Mask, "astype");
-    }
+  llvm::Type *SrcTy = Src->getType();
+  unsigned NumElementsSrc = isa<llvm::VectorType>(SrcTy) ?
+    cast<llvm::VectorType>(SrcTy)->getNumElements() : 0;
+  unsigned NumElementsDst = isa<llvm::VectorType>(DstTy) ?
+    cast<llvm::VectorType>(DstTy)->getNumElements() : 0;
+
+  // Going from vec3 to non-vec3 is a special case and requires a shuffle
+  // 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);
+    Src = Builder.CreateBitCast(Src, DstTy);
+    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) {
+    auto Vec4Ty = llvm::VectorType::get(DstTy->getVectorElementType(), 4);
+    Src = Builder.CreateBitCast(Src, Vec4Ty);
+    Src = ConvertVec3AndVec4(Builder, CGF, Src, 3);
+    Src->setName("astype");
+    return Src;
   }
 
   return Builder.CreateBitCast(Src, DstTy, "astype");
 }
 
 Value *ScalarExprEmitter::VisitAtomicExpr(AtomicExpr *E) {
   return CGF.EmitAtomicExpr(E).getScalarVal();
 }

cfe/trunk/test/CodeGenOpenCL/as_type.cl

+// RUN: %clang_cc1 %s -emit-llvm -triple spir-unknown-unknown -o - | FileCheck %s
+
+typedef __attribute__(( ext_vector_type(3) )) char char3;
+typedef __attribute__(( ext_vector_type(4) )) char char4;
+typedef __attribute__(( ext_vector_type(16) )) char char16;
+typedef __attribute__(( ext_vector_type(3) )) int int3;
+
+//CHECK: define spir_func <3 x i8> @f1(<4 x i8> %[[x:.*]])
+//CHECK: %[[astype:.*]] = shufflevector <4 x i8> %[[x]], <4 x i8> undef, <3 x i32> <i32 0, i32 1, i32 2>
+//CHECK: ret <3 x i8> %[[astype]]
+char3 f1(char4 x) {
+  return  __builtin_astype(x, char3);
+}
+
+//CHECK: define spir_func <4 x i8> @f2(<3 x i8> %[[x:.*]])
+//CHECK: %[[astype:.*]] = shufflevector <3 x i8> %[[x]], <3 x i8> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
+//CHECK: ret <4 x i8> %[[astype]]
+char4 f2(char3 x) {
+  return __builtin_astype(x, char4);
+}
+
+//CHECK: define spir_func <3 x i8> @f3(i32 %[[x:.*]])
+//CHECK: %[[cast:.*]] = bitcast i32 %[[x]] to <4 x i8>
+//CHECK: %[[astype:.*]] = shufflevector <4 x i8> %[[cast]], <4 x i8> undef, <3 x i32> <i32 0, i32 1, i32 2>
+//CHECK: ret <3 x i8> %[[astype]]
+char3 f3(int x) {
+  return __builtin_astype(x, char3);
+}
+
+//CHECK: define spir_func <4 x i8> @f4(i32 %[[x:.*]])
+//CHECK: %[[astype:.*]] = bitcast i32 %[[x]] to <4 x i8>
+//CHECK-NOT: shufflevector
+//CHECK: ret <4 x i8> %[[astype]]
+char4 f4(int x) {
+  return __builtin_astype(x, char4);
+}
+
+//CHECK: define spir_func i32 @f5(<3 x i8> %[[x:.*]])
+//CHECK: %[[shuffle:.*]] = shufflevector <3 x i8> %[[x]], <3 x i8> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 undef>
+//CHECK: %[[astype:.*]] = bitcast <4 x i8> %[[shuffle]] to i32
+//CHECK: ret i32 %[[astype]]
+int f5(char3 x) {
+  return __builtin_astype(x, int);
+}
+
+//CHECK: define spir_func i32 @f6(<4 x i8> %[[x:.*]])
+//CHECK: %[[astype]] = bitcast <4 x i8> %[[x]] to i32
+//CHECK-NOT: shufflevector
+//CHECK: ret i32 %[[astype]]
+int f6(char4 x) {
+  return __builtin_astype(x, int);
+}
+
+//CHECK: define spir_func <3 x i8> @f7(<3 x i8> %[[x:.*]])
+//CHECK-NOT: bitcast
+//CHECK-NOT: shufflevector
+//CHECK: ret <3 x i8> %[[x]]
+char3 f7(char3 x) {
+  return __builtin_astype(x, char3);
+}
+
+//CHECK: define spir_func <3 x i32> @f8(<16 x i8> %[[x:.*]])
+//CHECK: %[[cast:.*]] = bitcast <16 x i8> %[[x]] to <4 x i32>
+//CHECK: %[[astype:.*]] = shufflevector <4 x i32> %[[cast]], <4 x i32> undef, <3 x i32> <i32 0, i32 1, i32 2>
+//CHECK: ret <3 x i32> %[[astype]]
+int3 f8(char16 x) {
+  return __builtin_astype(x, int3);
+}

cfe/trunk/test/SemaOpenCL/as_type.cl

+// RUN: %clang_cc1 %s -emit-llvm -triple spir-unknown-unknown -o - -verify -fsyntax-only
+
+typedef __attribute__(( ext_vector_type(3) )) char char3;
+typedef __attribute__(( ext_vector_type(16) )) char char16;
+
+char3 f1(char16 x) {
+  return  __builtin_astype(x, char3); // expected-error{{invalid reinterpretation: sizes of 'char3' (vector of 3 'char' values) and 'char16' (vector of 16 'char' values) must match}}
+}
+
+char16 f3(int x) {
+  return __builtin_astype(x, char16); // expected-error{{invalid reinterpretation: sizes of 'char16' (vector of 16 'char' values) and 'int' must match}}
+}
+