Page MenuHomePhabricator

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

Authored by george.burgess.iv on Dec 22 2015, 11:17 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

george.burgess.iv retitled this revision from to [Sema] Fix ICE on casting a vector of bools to a vector of T.
george.burgess.iv updated this object.
george.burgess.iv added a reviewer: Anastasia.
george.burgess.iv added a subscriber: cfe-commits.
This comment was removed by george.burgess.iv.
Anastasia edited edge metadata.Jan 25 2016, 9:49 AM

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 edited edge metadata.
george.burgess.iv marked an inline comment as done.
  • 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

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

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 marked an inline comment as done.

Updated comment, as requested.

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 added a reviewer: hfinkel.

+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 accepted this revision.Jan 27 2016, 3:52 AM
Anastasia edited edge metadata.

Seems sensible. LGTM!

This revision is now accepted and ready to land.Jan 27 2016, 3:52 AM
This revision was automatically updated to reflect the committed changes.