improved vector bool/pixel support

Hi all

The patch enables support for 'vector bool ...' and makes clang distinguish between 'vector pixel' and 'vector unsigned int'. Please review.

vector_pixel_bool.patch (25.4 KB)

Hi Anton,

My understanding is that 'vector bool' and 'vector pixel' are exclusive. If so, the argument to getVectorType should be an enum instead of 3 bools (altivec, ispixel, isbool). Can you refactor it to take an enum?

-Chris

Hi Chris,

Yes, 'bool' and 'pixel' are not allowed to be placed together, but they are of a slightly different nature - 'pixel' is a type specifier and 'bool' is not, type specifier should be specified after 'vector bool'.
Sending you the fresh copy of the original patch and the refactored version with enum used.

vector_pixel+bool_original.patch (24.9 KB)

vector_pixel+bool_refactored.patch (26.4 KB)

I applied the patch with several minor changes (e.g. fitting in 80 columns) here: r106619. Note that the testcases didn't pass with the test applied. I tweaked them to make them work, please verify that the test changes are what you intend. Having the diagnostics print __pixel instead of pixel seems dubious. Also, please make sure that future patches pass tests before sending in the patch.

However, though I applied the patch I don't think that this hunk is right:

@@ -1529,16 +1529,19 @@
   // If the element type isn't canonical, this won't be a canonical type either,
   // so fill in the canonical type field.
   QualType Canonical;
- if (!vecType.isCanonical() || IsAltiVec || IsPixel) {
- Canonical = getVectorType(getCanonicalType(vecType),
- NumElts, false, false);
+ if (!vecType.isCanonical() || (AltiVecSpec == VectorType::AltiVec)) {
+ // pass VectorType::NotAltiVec for AltiVecSpec to make AltiVec canonical
+ // vector type (except 'vector bool ...' and 'vector Pixel') the same as
+ // the equivalent GCC vector types
+ Canonical = getVectorType(getCanonicalType(vecType), NumElts,
+ VectorType::NotAltiVec);

The canonical type for the vector should not change its underlying structure, it should just remove "sugar". Doug, if you could, I'd appreciate it if you could look at this and see if there is a better way to get the vector type compatibility stuff that this code seems to be going for. I'm also ok with temporarily removing this hunk until a proper solution is found, please advise.

-Chris

   

My understanding is that 'vector bool' and 'vector pixel' are exclusive. If so, the argument to getVectorType should be an enum instead of 3 bools (altivec, ispixel, isbool). Can you refactor it to take an enum?

-Chris

Hi Chris,

Yes, 'bool' and 'pixel' are not allowed to be placed together, but they are of a slightly different nature - 'pixel' is a type specifier and 'bool' is not, type specifier should be specified after 'vector bool'.
Sending you the fresh copy of the original patch and the refactored version with enum used.
     

I applied the patch with several minor changes (e.g. fitting in 80 columns) here: r106619. Note that the testcases didn't pass with the test applied. I tweaked them to make them work, please verify that the test changes are what you intend. Having the diagnostics print __pixel instead of pixel seems dubious. Also, please make sure that future patches pass tests before sending in the patch.

Sent unverified test versions, sorry.

However, though I applied the patch I don't think that this hunk is right:

@@ -1529,16 +1529,19 @@
    // If the element type isn't canonical, this won't be a canonical type either,
    // so fill in the canonical type field.
    QualType Canonical;
- if (!vecType.isCanonical() || IsAltiVec || IsPixel) {
- Canonical = getVectorType(getCanonicalType(vecType),
- NumElts, false, false);
+ if (!vecType.isCanonical() || (AltiVecSpec == VectorType::AltiVec)) {
+ // pass VectorType::NotAltiVec for AltiVecSpec to make AltiVec canonical
+ // vector type (except 'vector bool ...' and 'vector Pixel') the same as
+ // the equivalent GCC vector types
+ Canonical = getVectorType(getCanonicalType(vecType), NumElts,
+ VectorType::NotAltiVec);

The canonical type for the vector should not change its underlying structure, it should just remove "sugar". Doug, if you could, I'd appreciate it if you could look at this and see if there is a better way to get the vector type compatibility stuff that this code seems to be going for. I'm also ok with temporarily removing this hunk until a proper solution is found, please advise.

-Chris

Eliminated changes to the canonical type, added compatibility checking instead - patch is attached. Verified changed tests and ensured that patch have not affected common test results.

Altivec_GCC_compat.patch (7.38 KB)

Oops, I missed Chris's original request for me to look at this. Anyway, I disagree with Chris: if the AltiVec vector types are meant to be identical to GNU vector types, then they need to have the same canonical type. The code that is currently in the tree looks like it does that properly already.

  - Doug

I'm happy to defer to Doug's opinion here. However, I thought that the idea of a canonical type was that it represent the structural behavior of the type. Semantically there should be no difference between the sugared and desugared type. If "altivec" vectors have more behavior than just how they print, I don't think it's just sugar, right?

-Chris

Do AltiVec vectors have more/different behavior from than GCC vectors? I honestly don't know. If they do, they need their own canonical types and we'll need to introduce appropriate implicit conversions between the two kinds of vectors. If the behavior is the same, then it's just sugar.

  - Doug

Yes, they do. For example,

my_altivec_vector x = {1} does a splat, not a zero fill.

-Chris

As I understand the conclusion is that AltiVec vectors need their own canonical type. So resending you the updated patch for review. The patch gives unique canonical type for AltiVec vectors and adds compatibility checking.

Altivec_GCC_compat.patch (7.38 KB)

Index: lib/AST/ASTContext.cpp

Added code for an implicit conversion between GCC and AltiVec vectors to Sema. Also attached tests with the relaxed vector conversions turned off. Please review.

Altivec_GCC_compat.patch (5.77 KB)

test.c (342 Bytes)

test.c (342 Bytes)

Index: lib/Sema/SemaExpr.cpp

Changed "return IncompatibleVectors" to "return Compatible".

Hmm.. The attribute is absolutely useless in both cases..
I used it in the C test to reach the IsVectorConversion() finction in SemaOverloaded.cpp, but there should be at least two overloaded candidates to reach the desired code.

Added "?:" operator to tests, no troubles.

Altivec_GCC_compat.patch (5.77 KB)

test.c (408 Bytes)

test.cpp (321 Bytes)

Looks great, thanks! Committed as r110437.

  - Doug