Page MenuHomePhabricator

Parse vector bool when stdbool.h and altivec.h are included
ClosedPublic

Authored by ZarkoCA on May 7 2021, 4:30 AM.

Details

Summary

Currently when including stdbool.h and altivec.h declaration of vector bool leads to
errors due to bool being expanded to '_Bool`. This patch allows the parser
to recognize _Bool.

Diff Detail

Event Timeline

ZarkoCA requested review of this revision.May 7 2021, 4:30 AM
ZarkoCA created this revision.
ZarkoCA edited the summary of this revision. (Show Details)May 7 2021, 4:30 AM
nemanjai added inline comments.
clang/test/Parser/altivec-bool.c
11 ↗(On Diff #343641)

Do we not want the test case to provide a similar define to what stdbool.h does?

ZarkoCA updated this revision to Diff 343656.May 7 2021, 5:33 AM
ZarkoCA edited the summary of this revision. (Show Details)
  • update test cases to contain _Bool as well
ZarkoCA added inline comments.May 7 2021, 9:17 AM
clang/test/Parser/altivec-bool.c
11 ↗(On Diff #343641)

Is using eg adding cases such as: __vector _Bool char bc; what you meant? Added those in just now.

This seems fine to me now but I'll defer to front end experts for approval.

clang/lib/Parse/ParseDecl.cpp
7347–7349

Minor nit: remove (inconsistently applied) extra parentheses.

clang/lib/Parse/Parser.cpp
516

No need for an extra if block. Just need to figure out which block above this should go in.

ZarkoCA updated this revision to Diff 344426.May 11 2021, 9:03 AM
ZarkoCA edited the summary of this revision. (Show Details)
ZarkoCA added reviewers: uweigand, neumannh.
  • clean up code per comments
  • Add test case for Z targets since that target is also affected
ZarkoCA marked 3 inline comments as done.May 11 2021, 9:04 AM
ZarkoCA edited reviewers, added: Everybody0523; removed: neumannh.May 11 2021, 9:15 AM

Minor nitpick, but is there a term that encompasses both the Z Vector syntax and altivec? Since the test checks both Z and PPC it's a little odd that the test's filename only references altivec.

This means the implementation now deviates from the documented vector extension syntax, right? I guess it's strictly an extension of the documented syntax, but that may still lead to incompatibilities with other compilers for the platform. If we want to make such a change, should it be synchronized with e.g. GCC, XL, etc. ?

This means the implementation now deviates from the documented vector extension syntax, right? I guess it's strictly an extension of the documented syntax, but that may still lead to incompatibilities with other compilers for the platform. If we want to make such a change, should it be synchronized with e.g. GCC, XL, etc. ?

GCC and XL already accept this syntax on Linux on Power and AIX.

For example this simple test case:

#include <stdbool.h>
#include <altivec.h>

vector bool char bc;

Can compile with GCC 9/10 and XLC 16.1 on Linux on Power. On AIX, GCC 8.3 on AIX and XLC 16.1 can also compile it successfully. Latest main trunk clang throws up an error on those platforms.

From offline conversation it looks like XLC on z/OS can also compile the test case. @Everybody0523 can confirm for sure.

Minor nitpick, but is there a term that encompasses both the Z Vector syntax and altivec? Since the test checks both Z and PPC it's a little odd that the test's filename only references altivec.

That's a good point, I can rename the file to altivec-zvector-bool.c?

This means the implementation now deviates from the documented vector extension syntax, right? I guess it's strictly an extension of the documented syntax, but that may still lead to incompatibilities with other compilers for the platform. If we want to make such a change, should it be synchronized with e.g. GCC, XL, etc. ?

GCC and XL already accept this syntax on Linux on Power and AIX.

For example this simple test case:

#include <stdbool.h>
#include <altivec.h>

vector bool char bc;

Can compile with GCC 9/10 and XLC 16.1 on Linux on Power. On AIX, GCC 8.3 on AIX and XLC 16.1 can also compile it successfully. Latest main trunk clang throws up an error on those platforms.

From offline conversation it looks like XLC on z/OS can also compile the test case. @Everybody0523 can confirm for sure.

Interesting. On Z using GCC I currently get this error:

vbool.c:2:1: error: invalid vector type for attribute ‘vector_size’
    2 | vector _Bool x;

But looking at the GCC sources, it seems we actually intended to support this use as well, there's just a bug. Given that, I think I'd be fine with adding this to LLVM -- I'll make sure the GCC bug gets fixed as well.

This means the implementation now deviates from the documented vector extension syntax, right? I guess it's strictly an extension of the documented syntax, but that may still lead to incompatibilities with other compilers for the platform. If we want to make such a change, should it be synchronized with e.g. GCC, XL, etc. ?

GCC and XL already accept this syntax on Linux on Power and AIX.

For example this simple test case:

#include <stdbool.h>
#include <altivec.h>

vector bool char bc;

Can compile with GCC 9/10 and XLC 16.1 on Linux on Power. On AIX, GCC 8.3 on AIX and XLC 16.1 can also compile it successfully. Latest main trunk clang throws up an error on those platforms.

From offline conversation it looks like XLC on z/OS can also compile the test case. @Everybody0523 can confirm for sure.

Interesting. On Z using GCC I currently get this error:

vbool.c:2:1: error: invalid vector type for attribute ‘vector_size’
    2 | vector _Bool x;

But looking at the GCC sources, it seems we actually intended to support this use as well, there's just a bug. Given that, I think I'd be fine with adding this to LLVM -- I'll make sure the GCC bug gets fixed as well.

Thank you for looking at GCC on Z. That was the only case where I didn't have information from.

xlc on z/OS will compile

vector _Bool char bc;

on any language level high enough to recognize _Bool as a valid keyword.

ZarkoCA updated this revision to Diff 344785.May 12 2021, 5:46 AM
  • Rename test

LGTM with comment.

clang/test/Parser/altivec-zvector-bool.c
21–24

I think having these as declarations of the names used above is beneficial because it confirms that the types formed using the bool spelling are compatible with the same using the _Bool spelling.

This revision is now accepted and ready to land.May 12 2021, 7:54 PM

Expected Z behavior LGTM

Everybody0523 accepted this revision.May 13 2021, 7:27 AM
This revision was landed with ongoing or failed builds.May 13 2021, 8:48 AM
This revision was automatically updated to reflect the committed changes.
jonpa added a subscriber: jonpa.May 18 2021, 7:36 AM

But looking at the GCC sources, it seems we actually intended to support this use as well, there's just a bug. Given that, I think I'd be fine with adding this to LLVM -- I'll make sure the GCC bug gets fixed as well.

I've fixed the GCC issue now: https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570561.html