[PATCH] Warn on use of vector initializers in ARM BE mode

[Alp Toker]

On 19/06/2014 13:13, James Molloy wrote:
> Hi Alp,
>
> Thanks for the comments. New patch attached, is it any better? I'm not sure
> exactly what style we normally use for the diagnostics.

Yes, looking better. With a big disclaimer -- I'm not too familiar with 
this corner of ARM sema (Tim?) -- a couple more comments follow..

> +#include "clang/Basic/TargetInfo.h"
>  #include "clang/Sema/Designator.h"
>  #include "clang/Sema/Lookup.h"
>  #include "clang/Sema/SemaInternal.h"
> @@ -1204,6 +1205,45 @@ void InitListChecker::CheckVectorType(const 
> InitializedEntity &Entity,
>        CheckSubElementType(ElementEntity, IList, elementType, Index,
>                            StructuredList, StructuredIndex);
>      }
> +
> +    llvm::Triple Triple = SemaRef.Context.getTargetInfo().getTriple();
> +    if (!VerifyOnly &&
> +        (Triple.getArch() == llvm::Triple::arm64_be ||
> +         Triple.getArch() == llvm::Triple::arm64 ||
> +         Triple.getArch() == llvm::Triple::armeb ||
> +         Triple.getArch() == llvm::Triple::arm)) {

How about dropping all that and just checking directly if the 
initialized entity has a Neon vector type? Something like this..

     if (VerifyOnly)
       return;

     const VectorType *T = Entity.getType()->getAs<VectorType>();
     if (T->getVectorKind() == VectorType::NeonVector ||
         T->getVectorKind() == VectorType::NeonPolyVector) {

> --- /dev/null
> +++ b/test/Sema/big-endian-neon-initializers.c
> @@ -0,0 +1,9 @@
> +// RUN: %clang_cc1 %s -triple arm64_be -target-feature +neon -verify 
> -fsyntax-only
> +// RUN: %clang_cc1 %s -triple armebv7 -target-cpu cortex-a8 -verify 
> -fsyntax-only
> +
> +#include <arm_neon.h>
> +

This test will need '-ffreestanding' in both RUN lines to pick up the 
right arm_neon.h.

Alp.


>
> Cheers,
>
> James
>
>> -----Original Message-----
>> From: Alp Toker [mailto:alp at nuanti.com]
>> Sent: 19 June 2014 10:12
>> To: James Molloy; Clang Commits
>> Subject: Re: [PATCH] Warn on use of vector initializers in ARM BE mode
>>
>>
>> On 19/06/2014 11:53, James Molloy wrote:
>>> Hi Alp,
>>>
>>> I certainly thought about this, however it's not exactly clear how to
>>> generate good fixits. I can generate fixits that "kind of work", but my
>>> understanding was that the rule of thumb is that we shouldn't generate a
>>> fixit unless we're pretty sure it's correct.
>>>
>>> The main problem is that we must create a fixit on all the usage sites.
>> The
>>> fix would be:
>>>
>>>     1. Convert the vector declaration into an array declaration.
>>>     2. Add vld* calls on each use of the declaration.
>>>
>>> Which is fairly difficult in itself (updating all uses of a decl is more
>>> difficult than just adding a warning on the decl itself), but also to
>>> maintain the semantics of the program we'd need to add vst* calls (store
>> the
>>> result back) whenever the declaration may be modified either by
>> arithmetic
>>> or an intrinsic.
>>>
>>> Because of this complexity, I felt it wasn't really worth the investment
>> to
>>> generate proper fixits.
>> Fair enough if you don't see a reasonably straightforward syntactic FixIt.
>>
>> The patch as-is does need some tweaking, some quick observations:
>>
>> If reporting a portability issue, it makes sense to report the issue on
>> either endianness, not just the big-endian targets where it's known to
>> cause trouble.
>>
>> Lowercase first letters for diag messages.
>>
>> "are part of the GNU vector extensions ..." wording feels out of place
>> compared to the usual terminology.
>>
>> Alp.
>>
>>
>>
>>> Cheers,
>>>
>>> James
>>>
>>>> -----Original Message-----
>>>> From: Alp Toker [mailto:alp at nuanti.com]
>>>> Sent: 19 June 2014 09:44
>>>> To: James Molloy; Clang Commits
>>>> Subject: Re: [PATCH] Warn on use of vector initializers in ARM BE mode
>>>>
>>>>
>>>> On 19/06/2014 10:58, James Molloy wrote:
>>>>> Hi,
>>>>>
>>>>> The ability to use vector initializer lists is a GNU vector extension
>>>>> and is unrelated to the NEON intrinsics in arm_neon.h. On little
>>>>> endian machines it works fine, however on big endian machines it
>>>>> exhibits surprising behaviour:
>>>>>
>>>>> uint32x2_t x = {42, 64};
>>>>>
>>>>> return vget_lane_u32(x, 0); // Will return 64.
>>>>>
>>>>> Because of this, explicitly call out that it is unsupported on big
>>>>> endian machines.
>>>>>
>>>>> This patch will emit the following warning in big-endian mode:
>>>>>
>>>>> test.c:3:15: warning: Vector initializers are part of the GNU vector
>>>>> extensions and are not compatible with NEON intrinsics on big endian
>>>>> ARM targets [-Wgnu]
>>>>>
>>>>> int32x4_t x = {0, 1, 2, 3};
>>>>>
>>>>> ^
>>>>>
>>>>> test.c:3:15: note: Consider using vld1q_s32() to initialize a vector
>>>>> from memory, or vcombine_s32(vcreate_s32(), vcreate_s32()) to
>>>>> initialize from integer constants
>>>>>
>>>>> 1 warning generated.
>>>>>
>>>>> It's fairly simple, is it OK to commit?
>>>>>
>>>> Hi James,
>>>>
>>>> Would it be a big step to generate FixIts instead of describing the
>>>> possible fixes in words?
>>>>
>>>> I think as a rule of thumb, if it's worth describing code changes in
>>>> diagnostics, it's worth attempting a FixIt.
>>>>
>>>> Alp.
>>>>
>>>>
>>>>> Cheers,
>>>>>
>>>>> James
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>> --
>>>> http://www.nuanti.com
>>>> the browser experts
>>>>
>>>
>>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>

-- 
http://www.nuanti.com
the browser experts