Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL] Allow neon attributes for ARM host #94229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Jun 3, 2024

We permit neon attributes in CUDA device code, but this permission was only for CUDA device code. The same permission makes sense for SYCL device code as well, especially now that neon attributes are used in glibc headers.

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

llvmbot commented Jun 3, 2024

@llvm/pr-subscribers-clang

Author: Harald van Dijk (hvdijk)

Changes

We permit neon attributes in CUDA device code, but this permission was only for CUDA device code. The same permission makes sense for SYCL device code as well, especially now that neon attributes are used in glibc headers.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaType.cpp (+6-6)
  • (added) clang/test/SemaSYCL/neon-attrs.cpp (+16)
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 7cec82c701028..a8a1463522d03 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8077,10 +8077,10 @@ static bool verifyValidIntegerConstantExpr(Sema &S, const ParsedAttr &Attr,
 /// match one of the standard Neon vector types.
 static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
                                      Sema &S, VectorKind VecKind) {
-  bool IsTargetCUDAAndHostARM = false;
-  if (S.getLangOpts().CUDAIsDevice) {
+  bool IsTargetDeviceAndHostARM = false;
+  if (S.getLangOpts().CUDAIsDevice || S.getLangOpts().SYCLIsDevice) {
     const TargetInfo *AuxTI = S.getASTContext().getAuxTargetInfo();
-    IsTargetCUDAAndHostARM =
+    IsTargetDeviceAndHostARM =
         AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
   }
 
@@ -8090,7 +8090,7 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
         S.Context.getTargetInfo().hasFeature("mve") ||
         S.Context.getTargetInfo().hasFeature("sve") ||
         S.Context.getTargetInfo().hasFeature("sme") ||
-        IsTargetCUDAAndHostARM) &&
+        IsTargetDeviceAndHostARM) &&
       VecKind == VectorKind::Neon) {
     S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
         << Attr << "'neon', 'mve', 'sve' or 'sme'";
@@ -8099,7 +8099,7 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
   }
   if (!(S.Context.getTargetInfo().hasFeature("neon") ||
         S.Context.getTargetInfo().hasFeature("mve") ||
-        IsTargetCUDAAndHostARM) &&
+        IsTargetDeviceAndHostARM) &&
       VecKind == VectorKind::NeonPoly) {
     S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
         << Attr << "'neon' or 'mve'";
@@ -8121,7 +8121,7 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
 
   // Only certain element types are supported for Neon vectors.
   if (!isPermittedNeonBaseType(CurType, VecKind, S) &&
-      !IsTargetCUDAAndHostARM) {
+      !IsTargetDeviceAndHostARM) {
     S.Diag(Attr.getLoc(), diag::err_attribute_invalid_vector_type) << CurType;
     Attr.setInvalid();
     return;
diff --git a/clang/test/SemaSYCL/neon-attrs.cpp b/clang/test/SemaSYCL/neon-attrs.cpp
new file mode 100644
index 0000000000000..ec79fa5033308
--- /dev/null
+++ b/clang/test/SemaSYCL/neon-attrs.cpp
@@ -0,0 +1,16 @@
+// Host compilation on ARM with neon enabled (no errors expected).
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature +neon -fsyntax-only -verify=quiet %s
+
+// Host compilation on ARM with neon disabled.
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature -neon -fsyntax-only -verify %s
+
+// Device compilation on ARM (no errors expected).
+// RUN: %clang_cc1 -triple spirv64 -aux-triple aarch64-linux-gnu -fsycl-is-device -fsyntax-only -verify=quiet %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'}}

@hvdijk hvdijk force-pushed the allow-neon-attributes branch from 13fb2b9 to 895f71d Compare June 3, 2024 14:35
gglin001 added a commit to gglin001/llvm-dpcpp that referenced this pull request Oct 8, 2024
@hvdijk hvdijk force-pushed the allow-neon-attributes branch from 895f71d to 481f6c5 Compare December 19, 2024 14:31
@hvdijk
Copy link
Contributor Author

hvdijk commented Dec 19, 2024

Rebased to address a conflict with changes that have gone in since I first submitted this, the PR remains otherwise identical and waiting to be reviewed.

We permit neon attributes in CUDA device code, but this permission was
only for CUDA device code. The same permission makes sense for SYCL
device code as well, especially now that neon attributes are used in
glibc headers.
@hvdijk hvdijk force-pushed the allow-neon-attributes branch from 481f6c5 to b1a5399 Compare December 23, 2024 13:20
@hvdijk
Copy link
Contributor Author

hvdijk commented Dec 23, 2024

The rebase apparently conflicted with #95224. In the test I added, I checked that I preserved the existing behaviour with -target-feature -neon, but that behaviour has since changed. Updated to remove that bit of the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants