[cfe-dev] OpenCL patch: vector literals (alternative patch)

[Tanya Lattner]

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

-Tanya

On Jul 7, 2011, at 4:41 PM, Tanya Lattner wrote:

> 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>
> 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).
>> 
>>> 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.
>> 
>> 
>> 
> 
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev