[PATCH] Add error checking to reject neon_vector_type attribute on targets without NEON
Aaron Ballman
aaron at aaronballman.com
Mon Sep 16 05:37:05 PDT 2013
One minor nit from me, otherwise LGTM!
> From: Artyom Skrobov <artyom.skrobov at arm.com>
> Date: Wed, 11 Sep 2013 14:50:53 +0100
> Subject: [PATCH] Add error checking to reject neon_vector_type attribute on targets without NEON
>
> ---
> include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
> lib/Basic/Targets.cpp | 3 +--
> lib/Sema/SemaType.cpp | 7 +++++++
> test/Analysis/misc-ps-arm.m | 2 +-
> test/CodeGen/arm-arguments.c | 4 ++--
> test/CodeGen/arm-asm-diag.c | 2 +-
> test/CodeGen/arm-asm-warn.c | 2 +-
> test/CodeGen/struct-init.c | 2 +-
> test/CodeGen/struct-matching-constraint.c | 2 +-
> test/CodeGenCXX/aarch64-mangle-neon-vectors.cpp | 2 +-
> test/CodeGenCXX/mangle-neon-vectors.cpp | 2 +-
> test/Sema/aarch64-neon-vector-types.c | 2 +-
> test/Sema/neon-vector-types-support.c | 5 +++++
> test/Sema/neon-vector-types.c | 3 ++-
> test/SemaCXX/neon-vector-types.cpp | 2 +-
> 15 files changed, 28 insertions(+), 14 deletions(-)
> create mode 100644 test/Sema/neon-vector-types-support.c
>
> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
> index 7f8184c..64a76f5 100644
> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> @@ -1782,6 +1782,8 @@ def err_attribute_too_few_arguments : Error<
> def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
> def err_attribute_bad_neon_vector_size : Error<
> "Neon vector size must be 64 or 128 bits">;
> +def err_attribute_neon_unsupported : Error<
> + "%0 attribute is not supported for this target">;
The error name could be more generalized since this is more of a
target-neutral concept.
> def err_aligned_attribute_argument_not_int : Error<
> "'aligned' attribute requires integer constant">;
> def err_alignas_attribute_wrong_decl_type : Error<
> diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp
> index 00431c2..8a6a405 100644
> --- a/lib/Basic/Targets.cpp
> +++ b/lib/Basic/Targets.cpp
> @@ -3700,8 +3700,7 @@ public:
> .Case("arm", true)
> .Case("softfloat", SoftFloat)
> .Case("thumb", IsThumb)
> - .Case("neon", FPU == NeonFPU && !SoftFloat &&
> - StringRef(getCPUDefineSuffix(CPU)).startswith("7"))
> + .Case("neon", FPU == NeonFPU && !SoftFloat)
> .Default(false);
> }
> // FIXME: Should we actually have some table instead of these switches?
> diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
> index 2acfb26..99ac25e 100644
> --- a/lib/Sema/SemaType.cpp
> +++ b/lib/Sema/SemaType.cpp
> @@ -4728,6 +4728,13 @@ static bool isPermittedNeonBaseType(QualType &Ty,
> static void HandleNeonVectorTypeAttr(QualType& CurType,
> const AttributeList &Attr, Sema &S,
> VectorType::VectorKind VecKind) {
> + // Target must have NEON
> + if (!S.Context.getTargetInfo().hasFeature("neon")) {
> + S.Diag(Attr.getLoc(), diag::err_attribute_neon_unsupported)
> + << Attr.getName();
> + Attr.setInvalid();
> + return;
> + }
> // Check the attribute arguments.
> if (Attr.getNumArgs() != 1) {
> S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments)
> diff --git a/test/Analysis/misc-ps-arm.m b/test/Analysis/misc-ps-arm.m
> index 1ff0f17..a9ea327 100644
> --- a/test/Analysis/misc-ps-arm.m
> +++ b/test/Analysis/misc-ps-arm.m
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -triple thumbv7-apple-ios0.0.0 -analyze -analyzer-checker=core -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks -Wno-objc-root-class %s
> +// RUN: %clang_cc1 -triple thumbv7-apple-ios0.0.0 -target-feature +neon -analyze -analyzer-checker=core -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks -Wno-objc-root-class %s
> // expected-no-diagnostics
>
> // <rdar://problem/11405978> - Handle casts of vectors to structs, and loading
> diff --git a/test/CodeGen/arm-arguments.c b/test/CodeGen/arm-arguments.c
> index d02a217..b6bac9a 100644
> --- a/test/CodeGen/arm-arguments.c
> +++ b/test/CodeGen/arm-arguments.c
> @@ -1,6 +1,6 @@
> // REQUIRES: arm-registered-target
> -// RUN: %clang_cc1 -triple armv7-apple-darwin9 -target-abi apcs-gnu -emit-llvm -w -o - %s | FileCheck -check-prefix=APCS-GNU %s
> -// RUN: %clang_cc1 -triple armv7-apple-darwin9 -target-abi aapcs -emit-llvm -w -o - %s | FileCheck -check-prefix=AAPCS %s
> +// RUN: %clang_cc1 -triple armv7-apple-darwin9 -target-feature +neon -target-abi apcs-gnu -emit-llvm -w -o - %s | FileCheck -check-prefix=APCS-GNU %s
> +// RUN: %clang_cc1 -triple armv7-apple-darwin9 -target-feature +neon -target-abi aapcs -emit-llvm -w -o - %s | FileCheck -check-prefix=AAPCS %s
>
> // APCS-GNU-LABEL: define signext i8 @f0()
> // AAPCS-LABEL: define arm_aapcscc signext i8 @f0()
> diff --git a/test/CodeGen/arm-asm-diag.c b/test/CodeGen/arm-asm-diag.c
> index 925de46..944a271 100644
> --- a/test/CodeGen/arm-asm-diag.c
> +++ b/test/CodeGen/arm-asm-diag.c
> @@ -1,5 +1,5 @@
> // REQUIRES: arm-registered-target
> -// RUN: not %clang_cc1 -triple armv7 %s -S -o /dev/null 2>&1 | FileCheck %s
> +// RUN: not %clang_cc1 -triple armv7 -target-feature +neon %s -S -o /dev/null 2>&1 | FileCheck %s
>
> // rdar://13446483
> typedef __attribute__((neon_vector_type(2))) long long int64x2_t;
> diff --git a/test/CodeGen/arm-asm-warn.c b/test/CodeGen/arm-asm-warn.c
> index 6112b19..a580700 100644
> --- a/test/CodeGen/arm-asm-warn.c
> +++ b/test/CodeGen/arm-asm-warn.c
> @@ -1,5 +1,5 @@
> // REQUIRES: arm-registered-target
> -// RUN: %clang_cc1 -triple armv7 %s -emit-llvm -o /dev/null
> +// RUN: %clang_cc1 -triple armv7 -target-feature +neon %s -emit-llvm -o /dev/null
>
> char bar();
>
> diff --git a/test/CodeGen/struct-init.c b/test/CodeGen/struct-init.c
> index 5273138..30834ac 100644
> --- a/test/CodeGen/struct-init.c
> +++ b/test/CodeGen/struct-init.c
> @@ -1,5 +1,5 @@
> // REQUIRES: arm-registered-target
> -// RUN: %clang_cc1 -S -triple armv7-apple-darwin %s -emit-llvm -o - | FileCheck %s
> +// RUN: %clang_cc1 -S -triple armv7-apple-darwin -target-feature +neon %s -emit-llvm -o - | FileCheck %s
>
> typedef struct _zend_ini_entry zend_ini_entry;
> struct _zend_ini_entry {
> diff --git a/test/CodeGen/struct-matching-constraint.c b/test/CodeGen/struct-matching-constraint.c
> index bdd11c8..dfc3014 100644
> --- a/test/CodeGen/struct-matching-constraint.c
> +++ b/test/CodeGen/struct-matching-constraint.c
> @@ -1,5 +1,5 @@
> // REQUIRES: arm-registered-target
> -// RUN: %clang_cc1 -S -emit-llvm -triple armv7a-apple-darwin %s -o /dev/null
> +// RUN: %clang_cc1 -S -emit-llvm -triple armv7a-apple-darwin -target-feature +neon %s -o /dev/null
> typedef unsigned short uint16_t;
> typedef __attribute__((neon_vector_type(8))) uint16_t uint16x8_t;
>
> diff --git a/test/CodeGenCXX/aarch64-mangle-neon-vectors.cpp b/test/CodeGenCXX/aarch64-mangle-neon-vectors.cpp
> index 86509a0..2514704 100644
> --- a/test/CodeGenCXX/aarch64-mangle-neon-vectors.cpp
> +++ b/test/CodeGenCXX/aarch64-mangle-neon-vectors.cpp
> @@ -1,5 +1,5 @@
> // REQUIRES: aarch64-registered-target
> -// RUN: %clang_cc1 -triple aarch64-none-linux-gnu %s -emit-llvm -o - | FileCheck %s
> +// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +neon %s -emit-llvm -o - | FileCheck %s
>
> typedef unsigned char uint8_t;
> typedef unsigned short uint16_t;
> diff --git a/test/CodeGenCXX/mangle-neon-vectors.cpp b/test/CodeGenCXX/mangle-neon-vectors.cpp
> index 793c898..249ec2e 100644
> --- a/test/CodeGenCXX/mangle-neon-vectors.cpp
> +++ b/test/CodeGenCXX/mangle-neon-vectors.cpp
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -triple arm-none-linux-gnueabi %s -emit-llvm -o - | FileCheck %s
> +// RUN: %clang_cc1 -triple arm-none-linux-gnueabi -target-feature +neon %s -emit-llvm -o - | FileCheck %s
>
> typedef float float32_t;
> typedef __fp16 float16_t;
> diff --git a/test/Sema/aarch64-neon-vector-types.c b/test/Sema/aarch64-neon-vector-types.c
> index f4d58ff..ddfafbc 100644
> --- a/test/Sema/aarch64-neon-vector-types.c
> +++ b/test/Sema/aarch64-neon-vector-types.c
> @@ -1,5 +1,5 @@
> // REQUIRES: aarch64-registered-target
> -// RUN: %clang_cc1 %s -triple aarch64-none-linux-gnu -fsyntax-only -verify
> +// RUN: %clang_cc1 %s -triple aarch64-none-linux-gnu -target-feature +neon -fsyntax-only -verify
>
> typedef float float32_t;
> typedef unsigned char poly8_t;
> diff --git a/test/Sema/neon-vector-types-support.c b/test/Sema/neon-vector-types-support.c
> new file mode 100644
> index 0000000..065e7a1
> --- /dev/null
> +++ b/test/Sema/neon-vector-types-support.c
> @@ -0,0 +1,5 @@
> +// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
> +
> +typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported for this target}}
> +typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported for this target}}
> +
> diff --git a/test/Sema/neon-vector-types.c b/test/Sema/neon-vector-types.c
> index d15a50d..d8dd412 100644
> --- a/test/Sema/neon-vector-types.c
> +++ b/test/Sema/neon-vector-types.c
> @@ -1,4 +1,5 @@
> -// RUN: %clang_cc1 %s -fsyntax-only -verify
> +// RUN: %clang_cc1 %s -triple armv7 -target-feature +neon -fsyntax-only -verify
> +// RUN: %clang_cc1 %s -triple armv8 -target-feature +neon -fsyntax-only -verify
>
> typedef float float32_t;
> typedef signed char poly8_t;
> diff --git a/test/SemaCXX/neon-vector-types.cpp b/test/SemaCXX/neon-vector-types.cpp
> index 336fcd4..c2953d7 100644
> --- a/test/SemaCXX/neon-vector-types.cpp
> +++ b/test/SemaCXX/neon-vector-types.cpp
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -fsyntax-only -verify "-triple" "thumbv7-apple-ios3.0.0" %s
> +// RUN: %clang_cc1 -fsyntax-only -verify "-triple" "thumbv7-apple-ios3.0.0" -target-feature +neon %s
> // rdar://9208404
>
> typedef int MP4Err;
> --
> 1.7.9.5
>
~Aaron
On Mon, Sep 16, 2013 at 4:36 AM, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:
> Re-posting, attaching the patch itself which is for review.
>
>
> -----Original Message-----
> From: Bernard Ogden
> Sent: 13 September 2013 11:02
> To: Artyom Skrobov; llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH] Add error checking to reject neon_vector_type attribute
> on targets without NEON
>
> Hi Artyom,
>
> LGTM - but I think you should have posted this one on cfe-commits.
>
> The removal of the "sophisticated string parsing" gave me some pause as I
> think that architecturally-illegal feature combinations should result in a
> warning, at a minimum - but I'm fairly sure that other illegal combinations
> are permitted and this specific check is clearly broken, so discarding it
> seems the pragmatic thing to do.
>
> Regards,
>
> Bernie
>
>> -----Original Message-----
>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
>> bounces at cs.uiuc.edu] On Behalf Of Artyom Skrobov
>> Sent: 12 September 2013 17:17
>> To: llvm-commits at cs.uiuc.edu
>> Subject: [PATCH] Add error checking to reject neon_vector_type
>> attribute on targets without NEON
>>
>> Hello,
>>
>> We have discovered that test/Sema/neon-vector-types.c did not specify a
>> target, therefore testing the NEON-related attributes on the host
>> platform (which, in many cases, wouldn't even be ARM!)
>>
>> In addition to correcting this oversight in the test, we're adding a
>> check that the NEON-related attributes aren't used on a target without
>> NEON.
>>
>> We're also removing the sophisticated string parsing from
>> ARMTargetInfo::hasFeature, on the grounds that it's 1) broken in cases
>> when target CPU name is not specified explicitly; 2) outdated by
>> disregarding ARMv8; 3) clearly in the wrong place -- analysing target
>> CPU capabilities belongs in ARMTargetInfo initialisation, but needs not
>> re-run every time a NEON-related feature is used.
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list