[OpenMP] Silently accept `neon_vector_type` for the offloading device #127439

pradt2 · 2025-02-17T05:57:23Z

This addresses issue #113094

github-actions · 2025-02-17T05:57:42Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-02-17T05:58:12Z

@llvm/pr-subscribers-clang

Author: None (pradt2)

Changes

This addresses issue #113094

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

2 Files Affected:

(modified) clang/lib/Sema/SemaType.cpp (+6-7)
(added) clang/test/Sema/bug113094.cpp (+6)

diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 1fa5239a597c8..0c5cfdbba8d3d 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8336,12 +8336,11 @@ 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) {
-    const TargetInfo *AuxTI = S.getASTContext().getAuxTargetInfo();
-    IsTargetCUDAAndHostARM =
-        AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
-  }
+  const TargetInfo *AuxTI = S.getASTContext().getAuxTargetInfo();
+  bool IsArm = AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
+
+  bool IsTargetCUDAAndHostARM = IsArm && S.getLangOpts().CUDAIsDevice;
+  bool IsTargetOpenMPDeviceAndHostARM = IsArm && S.getLangOpts().OpenMPIsTargetDevice;
 
   // Target must have NEON (or MVE, whose vectors are similar enough
   // not to need a separate attribute)
@@ -8376,7 +8375,7 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
 
   // Only certain element types are supported for Neon vectors.
   if (!isPermittedNeonBaseType(CurType, VecKind, S) &&
-      !IsTargetCUDAAndHostARM) {
+      !IsTargetCUDAAndHostARM && !IsTargetOpenMPDeviceAndHostARM) {
     S.Diag(Attr.getLoc(), diag::err_attribute_invalid_vector_type) << CurType;
     Attr.setInvalid();
     return;
diff --git a/clang/test/Sema/bug113094.cpp b/clang/test/Sema/bug113094.cpp
new file mode 100644
index 0000000000000..0900db9efb041
--- /dev/null
+++ b/clang/test/Sema/bug113094.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -fopenmp --offload-arch=sm_90 -nocudalib -target aarch64-unknown-linux-gnu -c -Xclang -verify %s
+// REQUIRES: aarch64-registered-target
+
+// expected-no-diagnostics
+
+typedef __attribute__ ((__neon_vector_type__ (4))) float __f32x4_t;

Fznamznon

Thank you for the fix! I added more reviewers with offloading/OpenMP background. Could you please also modify the title and the description of the PR so it says what was the problem and how it was fixed instead of simply saying it fixed this bug?

Fznamznon · 2025-02-17T09:47:07Z

clang/test/Sema/bug113094.cpp

@@ -0,0 +1,6 @@
+// RUN: %clang -fopenmp --offload-arch=sm_90 -nocudalib -target aarch64-unknown-linux-gnu -c -Xclang -verify %s


We don't usually use clang driver in front-end tests. Could you please create a run line using pure front-end, i.e. via %clang_cc1 ?
Also, since this is OpenMP related, could you please put the test to OpenMP/ directory?

Seems CUDA used to have a test for this but it was removed for some reason? I would just stash it in SemaOpenMP or OpenMP and copy one of the existing -verify tests.

Fznamznon · 2025-02-17T09:52:13Z

clang/lib/Sema/SemaType.cpp

+  const TargetInfo *AuxTI = S.getASTContext().getAuxTargetInfo();
+  bool IsArm = AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
+
+  bool IsTargetCUDAAndHostARM = IsArm && S.getLangOpts().CUDAIsDevice;


This is more of a question to offloading experts of the community. I think all single-source offloading models eventually run into similar problems. It doesn't depend on the language being used (CUDA/OpenMP/SYCL) and what are the architectures. So I wonder if it makes sense to do a more generalized check, something like "if device doesn't support but the host does, dont error out".

I agree, unfortunately a lot of this handling is spread out all over the place. There's #126956 up which also tries to clean some of this up. However, right now I'd say that would be a larger cleanup out of scope for this bug fix.

jhuber6 · 2025-02-17T13:39:04Z

The test you added doesn't seem to pass. You can run it locally with llvm-lit /path/to/your/test -v.

github-actions · 2025-02-17T13:40:54Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

github-actions · 2025-02-17T13:43:07Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff d841c8842e17b7e74c3ee98c13a8a2505566deed 55e4d444572da286fdd4218178c6bc03c3e56b5b --extensions cpp -- clang/test/Sema/bug113094.cpp clang/lib/Sema/SemaType.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 0c5cfdbba8..9aeab7595d 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8337,10 +8337,12 @@ static bool verifyValidIntegerConstantExpr(Sema &S, const ParsedAttr &Attr,
 static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
                                      Sema &S, VectorKind VecKind) {
   const TargetInfo *AuxTI = S.getASTContext().getAuxTargetInfo();
-  bool IsArm = AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
+  bool IsArm =
+      AuxTI && (AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM());
 
   bool IsTargetCUDAAndHostARM = IsArm && S.getLangOpts().CUDAIsDevice;
-  bool IsTargetOpenMPDeviceAndHostARM = IsArm && S.getLangOpts().OpenMPIsTargetDevice;
+  bool IsTargetOpenMPDeviceAndHostARM =
+      IsArm && S.getLangOpts().OpenMPIsTargetDevice;
 
   // Target must have NEON (or MVE, whose vectors are similar enough
   // not to need a separate attribute)

shiltian · 2025-02-17T16:58:16Z

clang/test/Sema/bug113094.cpp

@@ -0,0 +1,6 @@
+// RUN: %clang -fopenmp --offload-arch=sm_90 -nocudalib -target aarch64-unknown-linux-gnu -c -Xclang -verify %s


Use %clang_cc1 here instead of the driver.

pradt2 · 2025-02-19T21:51:18Z

While I was working on the tests, I realised I can no longer replicate this issue in main. I guess it got fixed by someone, somewhere. I'm gonna close this now.

jhuber6 · 2025-02-19T21:52:40Z

While I was working on the tests, I realised I can no longer replicate this issue in main. I guess it got fixed by someone, somewhere. I'm gonna close this now.

I'm not aware of any changes in this area, but if it doesn't fail anymore I guess that's fine.

pradt2 added 3 commits February 16, 2025 00:22

[clang]: fix __neon_vector__type support when using OpenMP offloading

cfea753

[clang]: fix __neon_vector__type support when using OpenMP offloading

c50c5a9

[clang]: fix __neon_vector__type support when using OpenMP offloading

55e4d44

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 17, 2025

pradt2 mentioned this pull request Feb 17, 2025

Offloading breaks compilation on Aarch64 #113094

Closed

Fznamznon requested review from jhuber6 and shiltian February 17, 2025 09:45

Fznamznon reviewed Feb 17, 2025

View reviewed changes

jhuber6 changed the title ~~Clang/bug113094~~ [OpenMP] Silencly accept neon_vector_type for the offloading device Feb 17, 2025

jhuber6 changed the title ~~[OpenMP] Silencly accept neon_vector_type for the offloading device~~ [OpenMP] Silently accept neon_vector_type for the offloading device Feb 17, 2025

jhuber6 mentioned this pull request Feb 17, 2025

[clang][flang][Triple][llvm] Add isOffload function to LangOpts and isGPU function to Triple #126956

Merged

shiltian reviewed Feb 17, 2025

View reviewed changes

pradt2 closed this Feb 19, 2025

		@@ -0,0 +1,6 @@
		// RUN: %clang -fopenmp --offload-arch=sm_90 -nocudalib -target aarch64-unknown-linux-gnu -c -Xclang -verify %s

[OpenMP] Silently accept neon_vector_type for the offloading device #127439

[OpenMP] Silently accept neon_vector_type for the offloading device #127439

Uh oh!

Conversation

pradt2 commented Feb 17, 2025

Uh oh!

github-actions bot commented Feb 17, 2025

Uh oh!

llvmbot commented Feb 17, 2025

Uh oh!

Fznamznon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

Fznamznon Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 commented Feb 17, 2025

Uh oh!

github-actions bot commented Feb 17, 2025

Uh oh!

github-actions bot commented Feb 17, 2025

Uh oh!

shiltian Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

pradt2 commented Feb 19, 2025

Uh oh!

jhuber6 commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[OpenMP] Silently accept `neon_vector_type` for the offloading device #127439

[OpenMP] Silently accept `neon_vector_type` for the offloading device #127439

Fznamznon left a comment •

edited

Loading