[Clang] Remove preprocessor guards and global feature checks for NEON #95224

Lukacma · 2024-06-12T11:29:01Z

To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in ACLE.

llvmbot · 2024-06-12T11:29:37Z

@llvm/pr-subscribers-clang

Author: None (Lukacma)

Changes

To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in ACLE.

Full diff: https://github.com/llvm/llvm-project/pull/95224.diff

4 Files Affected:

(modified) clang/lib/Sema/SemaType.cpp (+8-10)
(modified) clang/test/Sema/arm-vector-types-support.c (+6-5)
(removed) clang/test/SemaCUDA/neon-attrs.cu (-22)
(modified) clang/utils/TableGen/NeonEmitter.cpp (-5)

diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 441fdcca0758f..9c0d043725dde 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8086,23 +8086,21 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
 
   // Target must have NEON (or MVE, whose vectors are similar enough
   // not to need a separate attribute)
-  if (!(S.Context.getTargetInfo().hasFeature("neon") ||
-        S.Context.getTargetInfo().hasFeature("mve") ||
-        S.Context.getTargetInfo().hasFeature("sve") ||
-        S.Context.getTargetInfo().hasFeature("sme") ||
+  if (!(S.Context.getTargetInfo().hasFeature("mve") ||
         IsTargetCUDAAndHostARM) &&
-      VecKind == VectorKind::Neon) {
+      VecKind == VectorKind::Neon && 
+      S.Context.getTargetInfo().getTriple().isArmMClass()) {
     S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
-        << Attr << "'neon', 'mve', 'sve' or 'sme'";
+        << Attr << "'mve'";
     Attr.setInvalid();
     return;
   }
-  if (!(S.Context.getTargetInfo().hasFeature("neon") ||
-        S.Context.getTargetInfo().hasFeature("mve") ||
+  if (!(S.Context.getTargetInfo().hasFeature("mve") ||
         IsTargetCUDAAndHostARM) &&
-      VecKind == VectorKind::NeonPoly) {
+      VecKind == VectorKind::NeonPoly &&
+      S.Context.getTargetInfo().getTriple().isArmMClass()) {
     S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
-        << Attr << "'neon' or 'mve'";
+        << Attr << "'mve'";
     Attr.setInvalid();
     return;
   }
diff --git a/clang/test/Sema/arm-vector-types-support.c b/clang/test/Sema/arm-vector-types-support.c
index ed5f5ba175a94..e648d791a2687 100644
--- a/clang/test/Sema/arm-vector-types-support.c
+++ b/clang/test/Sema/arm-vector-types-support.c
@@ -1,7 +1,8 @@
-// RUN: %clang_cc1 %s -triple armv7 -fsyntax-only -verify
-// RUN: %clang_cc1 %s -triple aarch64 -fsyntax-only -verify
-// RUN: %clang_cc1 %s -triple aarch64 -target-feature -fp-armv8 -target-abi aapcs-soft -fsyntax-only -verify
+// RUN: %clang_cc1 %s -triple armv8.1m.main -fsyntax-only -verify
+// RUN: %clang_cc1 %s -triple aarch64 -fsyntax-only -verify=sve-type
+// RUN: %clang_cc1 %s -triple aarch64 -target-feature -fp-armv8 -target-abi aapcs-soft -fsyntax-only -verify=sve-type
 
-typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'neon', 'mve', 'sve' or 'sme'; specify an appropriate -march= or -mcpu=}}
-typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_polyvector_type(16))) unsigned char poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'mve'; specify an appropriate -march= or -mcpu=}}
 typedef __attribute__((arm_sve_vector_bits(256))) void nosveflag; // expected-error{{'arm_sve_vector_bits' attribute is not supported on targets missing 'sve'; specify an appropriate -march= or -mcpu=}}
+                                                                  // sve-type-error@-1{{'arm_sve_vector_bits' attribute is not supported on targets missing 'sve'; specify an appropriate -march= or -mcpu=}}
diff --git a/clang/test/SemaCUDA/neon-attrs.cu b/clang/test/SemaCUDA/neon-attrs.cu
deleted file mode 100644
index 129056741ac9a..0000000000000
--- a/clang/test/SemaCUDA/neon-attrs.cu
+++ /dev/null
@@ -1,22 +0,0 @@
-// CPU-side compilation on ARM with neon enabled (no errors expected).
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature +neon -aux-triple nvptx64 -x cuda -fsyntax-only -verify=quiet %s
-
-// CPU-side compilation on ARM with neon disabled.
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature -neon -aux-triple nvptx64 -x cuda -fsyntax-only -verify %s
-
-// GPU-side compilation on ARM (no errors expected).
-// RUN: %clang_cc1 -triple nvptx64 -aux-triple arm64-linux-gnu -fcuda-is-device -x cuda -fsyntax-only -verify=quiet %s
-
-// Regular C++ compilation on ARM with neon enabled (no errors expected).
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature +neon -x c++ -fsyntax-only -verify=quiet %s
-
-// Regular C++ compilation on ARM with neon disabled.
-// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature -neon -x c++ -fsyntax-only -verify %s
-
-// quiet-no-diagnostics
-typedef __attribute__((neon_vector_type(4))) float float32x4_t;
-// expected-error@-1 {{'neon_vector_type' attribute is not supported on targets missing 'neon', 'mve', 'sve' or 'sme'}}
-// expect
-typedef unsigned char poly8_t;
-typedef __attribute__((neon_polyvector_type(8))) poly8_t poly8x8_t;
-// expected-error@-1 {{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'}}
diff --git a/clang/utils/TableGen/NeonEmitter.cpp b/clang/utils/TableGen/NeonEmitter.cpp
index 56f1fdf9ef574..626031d38cf00 100644
--- a/clang/utils/TableGen/NeonEmitter.cpp
+++ b/clang/utils/TableGen/NeonEmitter.cpp
@@ -2370,10 +2370,6 @@ void NeonEmitter::run(raw_ostream &OS) {
         "Please use -mfloat-abi=softfp or -mfloat-abi=hard\"\n";
   OS << "#else\n\n";
 
-  OS << "#if !defined(__ARM_NEON)\n";
-  OS << "#error \"NEON support not enabled\"\n";
-  OS << "#else\n\n";
-
   OS << "#include <stdint.h>\n\n";
 
   OS << "#include <arm_bf16.h>\n";
@@ -2450,7 +2446,6 @@ void NeonEmitter::run(raw_ostream &OS) {
   OS << "#undef __ai\n\n";
   OS << "#endif /* if !defined(__ARM_NEON) */\n";
   OS << "#endif /* ifndef __ARM_FP */\n";
-  OS << "#endif /* __ARM_NEON_H */\n";
 }
 
 /// run - Read the records in arm_fp16.td and output arm_fp16.h.  arm_fp16.h

github-actions · 2024-06-12T11:31:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Lukacma · 2024-06-12T11:37:00Z

Due to me screwing up on git rebase I recreated the PR. This PR has fix for MVE

mgabka · 2024-06-12T20:32:42Z

clang/test/Sema/arm-vector-types-support.c


-typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'neon', 'mve', 'sve' or 'sme'; specify an appropriate -march= or -mcpu=}}
-typedef __attribute__((neon_polyvector_type(16))) short poly8x16_t; // expected-error{{'neon_polyvector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=}}
+typedef __attribute__((neon_vector_type(2))) int int32x2_t; // expected-error{{'neon_vector_type' attribute is not supported on targets missing 'mve'; specify an appropriate -march= or -mcpu=}}


this error message does not sound like the one we are expecting to see as that suggests that "neon_vector_type'' is only supported when target supports mve. What I believe is not true.

From the changes to SemaType.cpp it looks like you want to emit this error only when the requested target is an m class architecture, this should be reflected in the error message.

But I am surprised that the run line:
RUN: %clang_cc1 %s -triple aarch64 -fsyntax-only -verify=sve-type

still generates this error.

The original idea behind this error message was to preserve original behaviour of the code for MVE, as this patch is only changing NEON. I am thinking that rather than changing the error message to add note about m class architecture, I should revert it to its original form. What do you think ?

RUN: %clang_cc1 %s -triple aarch64 -fsyntax-only -verify=sve-type

This run line doesn't generate any error message for neon attributes but only for sve attribute, if that's what you are asking about.

sorry, it was some time since I was working on sema, i didn't realise that "-verify=sve-type" make it only apply to the errors tagged with "sve-type-error@".

Is the error message used anywhere else? if not I personally am not against changing the error message, to be specific to M class processors.

M-profile specific message added. The original error message was used elsewhere so couldn't be changed so I just created new one

mgabka · 2024-06-20T09:12:46Z

clang/lib/Sema/SemaType.cpp

-        S.Context.getTargetInfo().hasFeature("mve") ||
-        S.Context.getTargetInfo().hasFeature("sve") ||
-        S.Context.getTargetInfo().hasFeature("sme") ||
+  if (!(S.Context.getTargetInfo().hasFeature("mve") ||
        IsTargetCUDAAndHostARM) &&


it looks to me like the logic here can be simplified and use of this variable can be removed entirely.

After reading
8b0ea48

It looks to me like somebody already disabled these errors when targeting GPUs, so we could just simplify the logic and emit the errors only when user targets M class CPUs, what do you think?

I am not sure I follow. The commit you pointed me to adds this variable to the check ?

I am suggesting that in that patch we removed error emission when targeting GPU, since now we are restricting error emission to M class processors, I think the extra handling for GPUS can be removed.

Ah now I understand. What you are trying to see that for mve there is no reason to check for GPUs as they will never be target for this extension?

CarolineConcatto · 2024-06-21T12:19:05Z

clang/lib/Sema/SemaType.cpp

-    S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
-        << Attr << "'neon' or 'mve'";
+  if (!S.Context.getTargetInfo().hasFeature("mve") &&
+      VecKind == VectorKind::NeonPoly &&


Wha happens if we have the original test(git show ed2d497), but without the neon check:
if (!(S.Context.getTargetInfo().hasFeature("mve") || IsTargetCUDAAndHostARM)){

No that wouldn't work as we would still get missing attribute error on aarch64 targets, as they don't have mve enabled.

…llvm#95224) To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in [ACLE](ARM-software/acle#321).

fix for mve

d5caa1a

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 12, 2024

fix formatting

a7ffcc5

Lukacma requested review from CarolineConcatto, momchil-velikov and hassnaaHamdi June 12, 2024 11:37

mgabka reviewed Jun 12, 2024

View reviewed changes

Lukacma added 2 commits June 17, 2024 09:42

change error message

77a3e14

fix formatting

b870f65

mgabka reviewed Jun 20, 2024

View reviewed changes

Remove checks for GPU

7d1162d

CarolineConcatto reviewed Jun 21, 2024

View reviewed changes

Incorrectly removed IsTargetCUDA

24790e6

mgabka approved these changes Jun 24, 2024

View reviewed changes

Merge branch 'main' into neon-attribute

c1b8dde

Lukacma merged commit 8a46bbb into llvm:main Jun 25, 2024
7 checks passed

Lukacma deleted the neon-attribute branch June 25, 2024 15:19

davemgreen mentioned this pull request Jul 9, 2024

clang-19 git fail to build ffmpeg ff_dct_unquantize_h263_neon #98152

Closed

Lukacma mentioned this pull request Sep 23, 2024

arm_neon.h intrinsics should be target-gated, not preprocessor-gated #56480

Closed

hvdijk mentioned this pull request Dec 23, 2024

[SYCL] Allow neon attributes for ARM host #94229

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Remove preprocessor guards and global feature checks for NEON #95224

[Clang] Remove preprocessor guards and global feature checks for NEON #95224

Lukacma commented Jun 12, 2024

llvmbot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024 •

edited

Loading

Lukacma commented Jun 12, 2024

mgabka Jun 12, 2024

Lukacma Jun 13, 2024

mgabka Jun 14, 2024

Lukacma Jun 17, 2024

mgabka Jun 20, 2024

Lukacma Jun 20, 2024

mgabka Jun 20, 2024

Lukacma Jun 21, 2024

CarolineConcatto Jun 21, 2024

Lukacma Jun 21, 2024

[Clang] Remove preprocessor guards and global feature checks for NEON #95224

[Clang] Remove preprocessor guards and global feature checks for NEON #95224

Conversation

Lukacma commented Jun 12, 2024

llvmbot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024 • edited Loading

Lukacma commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 12, 2024 •

edited

Loading