[cfe-dev] OpenCL patch: vector literals

[Tanya Lattner]

On Jul 7, 2011, at 8:17 AM, Anton Lokhmotov wrote:

> 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