OpenCL patch: vector literals

Clang Frontend
1

Anton,

I do not this is all necessary. All the support already exists for all the valid test cases except the last one ((float4)( float )). One just has to modify ActOnCastOfParenListExpr which is now BuildVectorLiteral to handle the splat case. I have not modified our patch to match the recent changes in TOT, but will do shortly.

For the invalid cases, I don’t see why one needs to modify the parser, but the last invalid case does crash the compiler which shouldn’t ever happen.

Also, I’d really appreciate if you didn’t just remove regression tests. Yes, you are 100% right that the test violates the OpenCL spec (my mistake and haste in making a patch), but its catching a nasty compiler crash and I’d rather try to modify the test instead of just removing it. I’ve fixed this in TOT.

Lastly, please make sure any unrelated code is not included (ie. lib/Frontend/CompilerInvocation.cpp changes).

Thanks,
Tanya

2

Hi Tanya,

Many thanks for your prompt review!

All the support already exists for all the valid test cases
except the last one ((float4)( float )). One just has to modify
ActOnCastOfParenListExpr which is now BuildVectorLiteral to handle
the splat case. I have not modified our patch to match the recent
changes in TOT, but will do shortly.

We would be happy with minimal changes to Clang provided that the OpenCL
requirements are properly met, including handling invalid code. We
specifically avoided using/modifying/breaking the existing functionality for
AltiVec because the OpenCL syntax is quite different. Thus, we believe that
for OpenCL CompilerInvocation::setLangDefaults() should disable Opts.AltiVec
(and OTOH enable Opts.Bool - but sorry about that slipping into this patch).

Yes, you are 100% right that the test violates the OpenCL spec
(my mistake and haste in making a patch), but its catching a nasty
compiler crash and I'd rather try to modify the test instead of just
removing it. I've fixed this in TOT.

Unfortunately,

void foo( uchar8 x )
{
  uchar4 val[4] = {{(uchar4){x.lo}}};
}

is still invalid in OpenCL C. In particular, OpenCL doesn't allow using
braces {} in vector literals.

[Actually, I'm quite confused by this test. Since x.lo produces uchar4, the
cast is superfluous? And then it attempts to initialize an array of vectors
val from vector x.lo. But I actually would expect only val[0] to be
initialized (backed by my experiments with gcc). But if it's just ensuring
that the compiler doesn't crash, that's fine (in the AltiVec mode).]

For the invalid cases, I don't see why one needs to modify the parser,
but the last invalid case does crash the compiler which shouldn't ever
happen.

Could you please ensure that the patch is applied correctly? It works fine
with r134483. Specifically, this modification prevents the compiler from
crashing:

[11:17:50] Vitaly Lugovskiy: diff --git a/lib/Sema/SemaInit.cpp
b/lib/Sema/SemaInit.cpp
index 16ba2a2..17af145 100644
--- a/lib/Sema/SemaInit.cpp
+++ b/lib/Sema/SemaInit.cpp
@@ -769,7 +769,8 @@ void InitListChecker::CheckSubElementType(const
InitializedEntity &Entity,
   // subaggregate, brace elision is assumed and the initializer is
   // considered for the initialization of the first member of
   // the subaggregate.
- if (ElemType->isAggregateType() || ElemType->isVectorType()) {
+ if (!SemaRef.getLangOptions().OpenCL &&
+ (ElemType->isAggregateType() || ElemType->isVectorType())) {
     CheckImplicitInitList(Entity, IList, ElemType, Index, StructuredList,
                           StructuredIndex);
     ++StructuredIndex;

If you provide your modifications to support vector literals, we will run
our test cases with combinatorial coverage to ensure that the OpenCL
requirements are met.

Best wishes,
Anton.

3

Hi Tanya,

Many thanks for your prompt review!

All the support already exists for all the valid test cases
except the last one ((float4)( float )). One just has to modify
ActOnCastOfParenListExpr which is now BuildVectorLiteral to handle
the splat case. I have not modified our patch to match the recent
changes in TOT, but will do shortly.

We would be happy with minimal changes to Clang provided that the OpenCL
requirements are properly met, including handling invalid code. We
specifically avoided using/modifying/breaking the existing functionality for
AltiVec because the OpenCL syntax is quite different. Thus, we believe that
for OpenCL CompilerInvocation::setLangDefaults() should disable Opts.AltiVec
(and OTOH enable Opts.Bool - but sorry about that slipping into this patch).

I actually agree that OpenCL should not reply on the Altivec flag and its something we have disabled in our implementation. There aren't too
many places where this is a problem thankfully.

Yes, you are 100% right that the test violates the OpenCL spec
(my mistake and haste in making a patch), but its catching a nasty
compiler crash and I'd rather try to modify the test instead of just
removing it. I've fixed this in TOT.

Unfortunately,

void foo( uchar8 x )
{
uchar4 val[4] = {{(uchar4){x.lo}}};
}

is still invalid in OpenCL C. In particular, OpenCL doesn't allow using
braces {} in vector literals.

Well, its an array of uchar4's. So its using {} to initialize the array. Can you point me to where in the spec it says I can't do this?
I'm not familiar with this rule.

[Actually, I'm quite confused by this test. Since x.lo produces uchar4, the
cast is superfluous? And then it attempts to initialize an array of vectors
val from vector x.lo. But I actually would expect only val[0] to be
initialized (backed by my experiments with gcc). But if it's just ensuring
that the compiler doesn't crash, that's fine (in the AltiVec mode).]

The test is funky I agree, but it should be valid syntax. It came from another larger CL example, which I unfortunately don't have anymore.

For the invalid cases, I don't see why one needs to modify the parser,
but the last invalid case does crash the compiler which shouldn't ever
happen.

Could you please ensure that the patch is applied correctly? It works fine
with r134483. Specifically, this modification prevents the compiler from
crashing:

I was just saying that without your changes, and simply modifying BuildVectorLiteral to handle the splat case, there
would be a crash on one of the invalid cases. I'll try your change below.

[11:17:50] Vitaly Lugovskiy: diff --git a/lib/Sema/SemaInit.cpp
b/lib/Sema/SemaInit.cpp
index 16ba2a2..17af145 100644
--- a/lib/Sema/SemaInit.cpp
+++ b/lib/Sema/SemaInit.cpp
@@ -769,7 +769,8 @@ void InitListChecker::CheckSubElementType(const
InitializedEntity &Entity,
  // subaggregate, brace elision is assumed and the initializer is
  // considered for the initialization of the first member of
  // the subaggregate.
- if (ElemType->isAggregateType() || ElemType->isVectorType()) {
+ if (!SemaRef.getLangOptions().OpenCL &&
+ (ElemType->isAggregateType() || ElemType->isVectorType())) {
    CheckImplicitInitList(Entity, IList, ElemType, Index, StructuredList,
                          StructuredIndex);
    ++StructuredIndex;

If you provide your modifications to support vector literals, we will run
our test cases with combinatorial coverage to ensure that the OpenCL
requirements are met.

Alright. If you have additional test cases, please send me a patch with them for the tree. I think its great to have more test cases in TOT.

-Tanya

4

Yes, you are 100% right that the test violates the OpenCL spec
(my mistake and haste in making a patch), but its catching a nasty
compiler crash and I'd rather try to modify the test instead of just
removing it. I've fixed this in TOT.

Unfortunately,

void foo( uchar8 x )
{
uchar4 val[4] = {{(uchar4){x.lo}}};
}

is still invalid in OpenCL C. In particular, OpenCL doesn't allow using
braces {} in vector literals.

Well, its an array of uchar4's. So its using {} to initialize the array. Can you point me to where in the spec it says I can't do this?
I'm not familiar with this rule.

I don't know what the base language for OpenCL is supposed to be, but this is C99 compound literal syntax, and:

  [C99 6.5.2.5] All the semantic rules and constraints for initializer lists in 6.7.8 are applicable to compound literals.

So this should be legal if C99 is an acceptable base language for OpenCL and the following is valid:
  uchar4 vec = { x.lo };

[Actually, I'm quite confused by this test. Since x.lo produces uchar4, the
cast is superfluous? And then it attempts to initialize an array of vectors
val from vector x.lo. But I actually would expect only val[0] to be
initialized (backed by my experiments with gcc).

When an aggregate has any initializer at all, missing elements are implicitly
zero-initialized.

John.

5

I'm attaching an alternative patch which handles the missing vector splat case. It passes all the valid and invalid test cases you provided and I included those tests in the patch. It also uses the fix in SemaInit that you included which handles the crash for one invalid case.

Thanks,
Tanya

OpenCL-VectorLiterals.patch (2.79 KB)

6

Any other comments on this patch? I'd like to commit.

-Tanya

7

Seems reasonable to me, but needs a testcase.

-Chris

8

Hi Tanya,

We have merged your patch and ours. It seems to work fine for both valid
and invalid test cases.

John McCall wrote:

So this should be legal if C99 is an acceptable base language for
OpenCL and the following is valid:
  uchar4 vec = { x.lo };

OpenCL is a superset of a subset of C99, so the fist condition is met.
However, section 6.1.6 of the OpenCL specification says that only
(typen)(...) is valid syntax for initializing vector literals, not {...}.
So ideally Clang would reject it (but I appreciate this is undesirable for
legacy reasons).

Thanks,
Anton.

vector-literals.patch (9.03 KB)

9

Ah, the test cases somehow didn't end up in the patch, but they are the same as in Anton's patch.

-Tanya

10

I read this section as specifying a new kind of expression, the OpenCL
vector literal, rather than prohibiting forming a vector in other ways.
The standard doesn't lay down rules for initializing vectors as
aggregates, but as you say, we clearly already permit it and should
not go back on it.

I'll let Tanya review the merged patch.

John.

11

Anton,

You shouldn't need to add anything from your patch to mine to get the test cases to work. Why do you think this extra code is necessary? Do you have a test case that isn't in the invalid/valid files that fails?

-Tanya

12

Attaching patch that actually has the test cases that Anton wrote:

OpenCL-VectorLiterals1.patch (4.61 KB)

13

Hi Tanya,

You shouldn't need to add anything from your patch to mine to get the
test cases to work. Why do you think this extra code is necessary? Do
you have a test case that isn't in the invalid/valid files that fails?

I'm a bit surprised this test passes without our modifications:

((float4)(1.0f))++; // expected-error{{expression is not assignable}}

Also, I was in a hurry and didn't include invalid tests for explicit and
implicit conversions between vector types, e.g.

uint4 u = (uint4)(1);
int4 i = u; // invalid implicit conversion
int4 e = (int4)u; // invalid explicit conversion

Best regards,
Anton.

P.S. I won't be able to reply until early next week - apologies in advance.

14

Anton,

I'll commit what I have, and when you get more time next week, you can re-evaluate if the additional work is needed. This obviously doesn't have to be the final patch if there are indeed outstanding issues. Hopefully thats sounds fair to you.

-Tanya

15

Please review the attached patch for misc vector-related OpenCL fixes.

Thanks,
Anton.

misc-vector.patch (10.9 KB)

16

Anton,

Can you please provide a summary of what the fixes are? It helps when reviewing.

Thanks,
Tanya

17

Hi Tanya,

The tests say it all really. Clang currently (r137302):

1) uses the language options for AltiVec (wrongly as you agreed) and
LaxVectorConversions (wrongly because vector conversions are disallowed in
OpenCL [except between vectors of the same type, when they are effectively a
no-op]).

2) hence accepts invalid explicit conversions between vectors of different
types and sizes:

  uint4 u = (uint4)(1);
  int4 e = (int4)u; // expected-error{{invalid conversion between ext-vector
type 'int4' and 'uint4'}}
  uint3 u3 = (uint3)u; // expected-error{{invalid conversion between
ext-vector type 'uint3' and 'uint4'}}

3) accepts taking the address of a vector literal, e.g.

  return &((int4)(3,2,1,0)); // expected-error{{address expression must be
an lvalue}}

4) only handles cases when a constant vector in the program scope is
initialised with a single element or another vector, but not recursively
e.g.:
  const float4 f_1_3 = (float4)(1,(float3)(2,3,4)); // [wrong!] error:
initializer element is not a compile-time constant

These changes seem not to affect other test cases (including ones available
for AltiVec).

Many thanks,
Anton.

18

Clang currently accepts taking the address of a vector literal in the OpenCL
mode, e.g.

   return &((int4)(3,2,1,0)); // expected-error{{address expression must be
an lvalue}}

which is wrong (http://llvm.org/bugs/show_bug.cgi?id=10966).

Please review the attached patch that fixes this problem.

Thanks,
Anton.

literals_rvalues.patch (1.99 KB)

19

Hi Anton,

What about compound literals which are not of vector type? The OpenCL
specification is silent on these, which means we should defer to C99,
which states that they are lvalues.

Thanks,

20

Right. Ideally, this should just apply to OpenCL vector literals,
i.e. this should be an r-value:
  (int4) (3,2,1,0)
and this should be an l-value:
  (int4) {3,2,10}

John.