[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

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

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

5559

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

5559

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!

lib/Sema/SemaExpr.cpp

bool Sema::areLaxCompatibleVectorTypes(QualType srcTy, QualType destTy) {
   uint64_t srcLen, destLen;
   QualType srcEltTy, destEltTy;
   if (!breakDownVectorType(srcTy, srcLen, srcEltTy)) return false;
   if (!breakDownVectorType(destTy, destLen, destEltTy)) return false;
   
   // ASTContext::getTypeSize will return the size rounded up to a
   // power of 2, so instead of using that, we need to use the raw
   // element size multiplied by the element count.
-  uint64_t srcEltSize = Context.getTypeSize(srcEltTy);
-  uint64_t destEltSize = Context.getTypeSize(destEltTy);
-  
+  //
+  // We need to be careful about booleans though; they're lowered to i1s, but
+  // getTypeSize views them as i8s.
+  uint64_t srcEltSize =
+      srcEltTy->isBooleanType() ? 1 : Context.getTypeSize(srcEltTy);
+  uint64_t destEltSize =
+      destEltTy->isBooleanType() ? 1 : Context.getTypeSize(destEltTy);
   return (srcLen * srcEltSize == destLen * destEltSize);
 }
 
 /// Is this a legal conversion between two types, one of which is
 /// known to be a vector type?
 bool Sema::isLaxVectorConversion(QualType srcTy, QualType destTy) {
   assert(destTy->isVectorType() || srcTy->isVectorType());
   

test/CodeGenCXX/bool-vector-conversion.cpp

+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s
+//
+// In many parts of clang, we treat a bool as an i8, but we lower it to an i1.
+// This is usually fine, but it broke things with vectors (e.g. we deemed a cast
+// from a [4 x i1] to a [1 x i32] to be legal, and proceeded to crash shortly
+// afterward).
+
+// Nothing but OpenCL allows vectors of booleans.
+// CHECK-LABEL: @_Z4testv
+void test() {
+  typedef __attribute__((__ext_vector_type__(8))) bool CLVectorBool8;
+  typedef __attribute__((__ext_vector_type__(1))) unsigned char CLVectorInt1;
+
+  // CHECK: store <8 x i1> zeroinitializer
+  CLVectorBool8 bools = (CLVectorBool8)false;
+  // CHECK: store <1 x i8>
+  CLVectorInt1 ints = (CLVectorInt1)bools;
+  // CHECK: store <8 x i1>
+  bools = (CLVectorBool8)ints;
+
+  // Run through the code in CGExprConstant.
+  // CHECK: store <8 x i1> zeroinitializer
+  bools = (CLVectorBool8)(CLVectorInt1)(CLVectorBool8)false;
+}
+