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.

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

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

Merged
merged 7 commits into from
Jun 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d5caa1a
fix for mve
Lukacma Jun 12, 2024
a7ffcc5
fix formatting
Lukacma Jun 12, 2024
77a3e14
change error message
Lukacma Jun 17, 2024
b870f65
fix formatting
Lukacma Jun 17, 2024
7d1162d
Remove checks for GPU
Lukacma Jun 21, 2024
24790e6
Incorrectly removed IsTargetCUDA
Lukacma Jun 21, 2024
c1b8dde
Merge branch 'main' into neon-attribute
Lukacma Jun 25, 2024
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
20 changes: 8 additions & 12 deletions clang/lib/Sema/SemaType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8086,23 +8086,19 @@ 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) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

VecKind == VectorKind::Neon) {
S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
<< Attr << "'neon', 'mve', 'sve' or 'sme'";
VecKind == VectorKind::Neon &&
S.Context.getTargetInfo().getTriple().isArmMClass()) {
S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << 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) {
S.Diag(Attr.getLoc(), diag::err_attribute_unsupported)
<< Attr << "'neon' or 'mve'";
VecKind == VectorKind::NeonPoly &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

S.Context.getTargetInfo().getTriple().isArmMClass()) {
S.Diag(Attr.getLoc(), diag::err_attribute_unsupported) << Attr << "'mve'";
Attr.setInvalid();
return;
}
Expand Down
11 changes: 6 additions & 5 deletions clang/test/Sema/arm-vector-types-support.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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=}}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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=}}
22 changes: 0 additions & 22 deletions clang/test/SemaCUDA/neon-attrs.cu
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions clang/utils/TableGen/NeonEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
Loading