[Sema] Fix ICE on casting a vector of bools to a vector of T

[george.burgess.iv]

Summary

Clang generally treats booleans as 8-bit types, but lowers them to 1-bit types. This means we currently happily accept C++ code like:

typedef __attribute__((__ext_vector_type__(4))) bool BoolVector; 
typedef __attribute__((__ext_vector_type__(1))) int IntVector;

int main() {
  BoolVector bv;
  IntVector iv = (IntVector)bv;
}

...And lower it to a cast from a [4 x i1] to a [1 x i32]. Which gives us a nice greeting in the form of an ICE.

ISTM that bools are only a valid element type in OpenCL vectors. The spec doesn't outline a definitive size for a bool (only that it must be able to support a 0 or 1), so I'm assuming that lowering to a vector of i1 is fine.

[george.burgess.iv]

Ping :)

[george.burgess.iv]

This comment was removed by george.burgess.iv.

[george.burgess.iv]

Ping :)

[george.burgess.iv]

Ping :)

[Anastasia]

I am generally not clear about the scope of this patch. Is it intended for OpenCL or C++?

As for OpenCL, boolean vector types are not permitted (Table 6.2). Also conversions between vector types are disallowed in general (Section 6.2.2) but unfortunately Clang doesn't diagnose it properly at the moment. :(

test/CodeGenCXX/bool-vector-conversion.cpp

8 ↗

(On Diff #43463)

Can you explain why? Table 6.2 doesn't list it among allowed valid types and Table 6.4 only says it's a reserved data type, but doesn't allow to use in application code anyways (Section 6.1.4).

[george.burgess.iv]

• Added diagnostics that prohibit vectors of booleans in OpenCL
• Added tests for extended vectors of bools in C (previously existed only in C++)
• Clarified some comments

[george.burgess.iv]

I am generally not clear about the scope of this patch. Is it intended for OpenCL or C++?

Sorry for the lack of clarity; I was mildly confused about what parts of the OpenCL spec apply to extended vectors when compiling code as C/C++. I think I have a better understanding of the split now. :) More directly, this is targeted at extended vectors in C/C++.

After playing around and asking on IRC, it seems that, when compiled as C/C++, extended vectors are intended to allow more operations than they do in OpenCL. Specifically, we have test cases for an extended vector of bools (see test/CodeGen/convertvector.c), and we have test cases for casting between extended vector types (see test/Sema/ext_vector_casts.cpp).

Hopefully the revised patch is a bit more clear, as well.

test/CodeGenCXX/bool-vector-conversion.cpp

8 ↗

(On Diff #43463)

Marking this as done because I don't think this needs to conform to the OpenCL spec when compiling as C/C++, and we have test cases that use extended vectors of bools in C/C++.

[Anastasia]

OpenCL side seems fine! Thanks for adding OpenCL diagnostic btw. I think we will also need to forbid the invalid conversions too at some point.

lib/Sema/SemaExpr.cpp

5715 ↗

(On Diff #45930)

Ok, I see now. I think your code here makes sense.

I am just generally a bit confused about how it should work. If I look at a slightly different example:

typedef __attribute__((__ext_vector_type__(4))) bool BoolVector; 
typedef __attribute__((__ext_vector_type__(1))) int IntVector;

int main() {
  BoolVector bv;
  bool b=1;
  bv.s1 = b;
}

this generates in IR:

store <4 x i1> %2, <4 x i1>* %bv

seems a bit odd to see a store of 4 bits. Would it be changed by a middlend/backend then?

lib/Sema/SemaType.cpp

2191 ↗

(On Diff #45930)

Could you change (there can be differences depending on a language version):

under Section 6.1.4 -> OpenCL v2.0 s6.1.4

[george.burgess.iv]

Updated comment, as requested.

[george.burgess.iv]

I think we will also need to forbid the invalid conversions too at some point.

test/SemaOpenCL/vector_conv_invalid.cl checks that clang gives you errors converting e.g. a 4 x unsigned to a 4 x int. So, unless you were referring to something else, that seems to be covered already. :)

lib/Sema/SemaExpr.cpp

5715 ↗

(On Diff #45930)

That's a good question! Clang+LLVM seems to handle straight-line code with odd-sized vectors fine, and seem to produce working x86 binaries (though, as you can probably tell, I'm far from an expert on vectors, so I could certainly be wrong).

We apparently have an issue with function calls, though. When I run the executable produced by this:

typedef __attribute__((__ext_vector_type__(4))) _Bool BoolVector;
BoolVector flip(BoolVector bv) {
  BoolVector bv2 = (BoolVector)1;
  return bv2 ^ bv;
}

int main() {
  BoolVector bv = (BoolVector)0;
  bv = flip(bv);
  printf("bv[0] = %d, bv[1] = %d, bv[2] = %d, bv[3] = %d\n", bv[0], bv[1], bv[2], bv[3]);
}

I see "bv[0] = 1, bv[1] = 0, bv[2] = 0, bv[3] = 0". I'm pretty sure it should be printing all 1s, since that's what I get if I manually inline flip, or change the vector element type to e.g. char.

After poking around a bit, I think the problem may be in how we pack/unpack these bit vectors as arguments and returned values (the same problem occurs even if flip is marked always_inline). Will look into it more/file a bug later today.

lib/Sema/SemaType.cpp

2191 ↗

(On Diff #45930)

Nice catch.

[george.burgess.iv]

+hfinkel as a reviewer, because git said he originally added the tests I'm removing.

I played around a bit, and it turns out that we don't generate correct machine code for bitvectors, and we get even more ICEs if we try to pass 3-bit bitvectors around. Awkward. This prompted me to talk with Chandler, who noted that we have no ABI for bitvectors, and that they might be useful for vector selects. Clang apparently does not support vector selects at the moment.

So, until we either support vector selects or get an ABI for this, disabling extended vectors of bools is probably our best bet. :)

[Anastasia]

Seems sensible. LGTM!

cfe/trunk/lib/Sema/SemaType.cpp

QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
   return T;
 }
 
 /// \brief Build an ext-vector type.
 ///
 /// Run the required checks for the extended vector type.
 QualType Sema::BuildExtVectorType(QualType T, Expr *ArraySize,
                                   SourceLocation AttrLoc) {
-  // unlike gcc's vector_size attribute, we do not allow vectors to be defined
+  // Unlike gcc's vector_size attribute, we do not allow vectors to be defined
   // in conjunction with complex types (pointers, arrays, functions, etc.).
-  if (!T->isDependentType() &&
-      !T->isIntegerType() && !T->isRealFloatingType()) {
+  //
+  // Additionally, OpenCL prohibits vectors of booleans (they're considered a
+  // reserved data type under OpenCL v2.0 s6.1.4), we don't support selects
+  // on bitvectors, and we have no well-defined ABI for bitvectors, so vectors
+  // of bool aren't allowed.
+  if ((!T->isDependentType() && !T->isIntegerType() &&
+       !T->isRealFloatingType()) ||
+      T->isBooleanType()) {
     Diag(AttrLoc, diag::err_attribute_invalid_vector_type) << T;
     return QualType();
   }
 
   if (!ArraySize->isTypeDependent() && !ArraySize->isValueDependent()) {
     llvm::APSInt vecSize(32);
     if (!ArraySize->isIntegerConstantExpr(vecSize, Context)) {
       Diag(AttrLoc, diag::err_attribute_argument_type)
         << "ext_vector_type" << AANT_ArgumentIntegerConstant
         << ArraySize->getSourceRange();
       return QualType();
     }
 
-    // unlike gcc's vector_size attribute, the size is specified as the
+    // Unlike gcc's vector_size attribute, the size is specified as the
     // number of elements, not the number of bytes.
     unsigned vectorSize = static_cast<unsigned>(vecSize.getZExtValue());
 
     if (vectorSize == 0) {
       Diag(AttrLoc, diag::err_attribute_zero_size)
       << ArraySize->getSourceRange();
       return QualType();
     }

cfe/trunk/test/CodeGen/convertvector.c

 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -target-cpu corei7-avx -emit-llvm %s -o - | FileCheck %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin9 -target-cpu corei7-avx -emit-llvm -x c++ %s -o - | FileCheck %s
 
 typedef double vector8double __attribute__((__vector_size__(64)));
 typedef float  vector8float  __attribute__((__vector_size__(32)));
 typedef long   vector8long   __attribute__((__vector_size__(64)));
 typedef short  vector8short  __attribute__((__vector_size__(16)));
 typedef unsigned long   vector8ulong   __attribute__((__vector_size__(64)));
 typedef unsigned short  vector8ushort  __attribute__((__vector_size__(16)));
 
 #ifdef __cplusplus
-#define BOOL bool
-#else
-#define BOOL _Bool
-#endif
-
-typedef BOOL vector8bool __attribute__((__ext_vector_type__(8)));
-
-#ifdef __cplusplus
 extern "C" {
 #endif
 
 vector8float flt_trunc(vector8double x) {
   return __builtin_convertvector(x, vector8float);
   // CHECK-LABEL: @flt_trunc
   // CHECK: fptrunc <8 x double> %{{[^ ]}} to <8 x float>
 }
 
 vector8double flt_ext(vector8float x) {
   return __builtin_convertvector(x, vector8double);
   // CHECK-LABEL: @flt_ext
   // CHECK: fpext <8 x float> %{{[^ ]}} to <8 x double>
 }
 
-vector8bool flt_tobool(vector8float x) {
-  return __builtin_convertvector(x, vector8bool);
-  // CHECK-LABEL: @flt_tobool
-  // CHECK-NOT: fptoui <8 x float> %{{[^ ]}} to <8 x i1>
-  // CHECK: fcmp une <8 x float> %{{[^ ]}}, zeroinitializer
-}
-
 vector8long flt_tosi(vector8float x) {
   return __builtin_convertvector(x, vector8long);
   // CHECK-LABEL: @flt_tosi
   // CHECK: fptosi <8 x float> %{{[^ ]}} to <8 x i64>
 }
 
 vector8ulong flt_toui(vector8float x) {
   return __builtin_convertvector(x, vector8ulong);

 }
 
 vector8long int_sext(vector8short x) {
   return __builtin_convertvector(x, vector8long);
   // CHECK-LABEL: @int_sext
   // CHECK: sext <8 x i16> %{{[^ ]}} to <8 x i64>
 }
 
-vector8bool int_tobool(vector8short x) {
-  return __builtin_convertvector(x, vector8bool);
-  // CHECK-LABEL: @int_tobool
-  // CHECK-NOT: trunc <8 x i16> %{{[^ ]}} to <8 x i1>
-  // CHECK: icmp ne <8 x i16> %{{[^ ]}}, zeroinitializer
-}
-
 vector8float int_tofp(vector8short x) {
   return __builtin_convertvector(x, vector8float);
   // CHECK-LABEL: @int_tofp
   // CHECK: sitofp <8 x i16> %{{[^ ]}} to <8 x float>
 }
 
 vector8float uint_tofp(vector8ushort x) {
   return __builtin_convertvector(x, vector8float);

cfe/trunk/test/Sema/ext_vector_casts.c

 // RUN: %clang_cc1 -triple x86_64-apple-macos10.7.0 -fsyntax-only -verify -fno-lax-vector-conversions -Wconversion %s
 
+typedef __attribute__((ext_vector_type(8))) _Bool BoolVector; // expected-error {{invalid vector element type '_Bool'}}
+
 typedef __attribute__(( ext_vector_type(2) )) float float2;
 typedef __attribute__(( ext_vector_type(3) )) float float3;
 typedef __attribute__(( ext_vector_type(4) )) int int4;
 typedef __attribute__(( ext_vector_type(8) )) short short8;
 typedef __attribute__(( ext_vector_type(4) )) float float4;
 typedef float t3 __attribute__ ((vector_size (16)));
 typedef __typeof__(sizeof(int)) size_t;
 typedef unsigned long ulong2 __attribute__ ((ext_vector_type(2)));

cfe/trunk/test/SemaOpenCL/bool-vectors.cl

+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+typedef __attribute__((ext_vector_type(16))) _Bool bool8; // expected-error{{invalid vector element type 'bool'}}