Skip to content

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

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.

Sign up for GitHub

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

Closed
pradt2 wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cfea753
[clang]: fix __neon_vector__type support when using OpenMP offloading
pradt2 Feb 16, 2025
c50c5a9
[clang]: fix __neon_vector__type support when using OpenMP offloading
pradt2 Feb 16, 2025
55e4d44
[clang]: fix __neon_vector__type support when using OpenMP offloading
pradt2 Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions clang/lib/Sema/SemaType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@Fznamznon Fznamznon Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

@jhuber6 jhuber6 Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bool IsTargetOpenMPDeviceAndHostARM = IsArm && S.getLangOpts().OpenMPIsTargetDevice;

// Target must have NEON (or MVE, whose vectors are similar enough
// not to need a separate attribute)
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions clang/test/Sema/bug113094.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// RUN: %clang -fopenmp --offload-arch=sm_90 -nocudalib -target aarch64-unknown-linux-gnu -c -Xclang -verify %s
Copy link
Contributor

@Fznamznon Fznamznon Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jhuber6 jhuber6 Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@shiltian shiltian Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use %clang_cc1 here instead of the driver.

// REQUIRES: aarch64-registered-target

// expected-no-diagnostics

typedef __attribute__ ((__neon_vector_type__ (4))) float __f32x4_t;
Loading