Skip to content

[AArch64][SME] Add diagnostics for SME attributes on lambda functions #121777

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

Merged
merged 4 commits into from
Jan 10, 2025
Merged

[AArch64][SME] Add diagnostics for SME attributes on lambda functions #121777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00772b8
[AArch64][SME] Add diagnostics to CheckConstexprFunctionDefinition
kmclaughlin-arm Dec 31, 2024
bbc80c2
- Move diagnostics to SemaARM.cpp
kmclaughlin-arm Jan 6, 2025
02658f7
- Mark CheckSMEFunctionDefAttributes as static
kmclaughlin-arm Jan 6, 2025
d47933c
- Removed static from CheckSMEFunctionDefAttributes
kmclaughlin-arm Jan 8, 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
2 changes: 2 additions & 0 deletions clang/include/clang/Sema/SemaARM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ class SemaARM : public SemaBase {
void handleNewAttr(Decl *D, const ParsedAttr &AL);
void handleCmseNSEntryAttr(Decl *D, const ParsedAttr &AL);
void handleInterruptAttr(Decl *D, const ParsedAttr &AL);

void CheckSMEFunctionDefAttributes(const FunctionDecl *FD);
};

SemaARM::ArmStreamingType getArmStreamingFnType(const FunctionDecl *FD);
Expand Down
53 changes: 53 additions & 0 deletions clang/lib/Sema/SemaARM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1328,4 +1328,57 @@ void SemaARM::handleInterruptAttr(Decl *D, const ParsedAttr &AL) {
ARMInterruptAttr(getASTContext(), AL, Kind));
}

// Check if the function definition uses any AArch64 SME features without
// having the '+sme' feature enabled and warn user if sme locally streaming
// function returns or uses arguments with VL-based types.
void SemaARM::CheckSMEFunctionDefAttributes(const FunctionDecl *FD) {
Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Jan 6, 2025

Choose a reason for hiding this comment

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

Should this be a static member function instead? (would have to take the ASTContext as parameter)

Copy link
Contributor Author

@kmclaughlin-arm kmclaughlin-arm Jan 7, 2025

Choose a reason for hiding this comment

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

I've changed this to static and added Sema as a parameter, so that the function can still use Diag() for emitting the diagnostics.

Copy link
Contributor

@cor3ntin cor3ntin Jan 7, 2025

Choose a reason for hiding this comment

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

It's a bit inconsistent with everything else, the intent is to use ARM().CheckSMEFunctionDefAttributes(FD)
(The reason we did that was to split Sema into multiple classes/files)

@Endilll

Copy link
Contributor

@Endilll Endilll Jan 7, 2025

Choose a reason for hiding this comment

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

It seems like an ARM-specific function, so you correctly placed it in SemaARM, and it's referenced in SemaDecl and SemaDeclCXX, so it can't be static. Corentin is correct that ARM().CheckSMEFunctionDefAttributes(FD) is the way to use it from outside of SemaARM.

The current state of the changes seems correct to me.

Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Jan 9, 2025

Choose a reason for hiding this comment

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

Thanks for your feedback, I didn't realise that there was already precedent for using ARM().Check...

const auto *Attr = FD->getAttr<ArmNewAttr>();
bool UsesSM = FD->hasAttr<ArmLocallyStreamingAttr>();
bool UsesZA = Attr && Attr->isNewZA();
bool UsesZT0 = Attr && Attr->isNewZT0();

if (FD->hasAttr<ArmLocallyStreamingAttr>()) {
if (FD->getReturnType()->isSizelessVectorType())
Diag(FD->getLocation(),
diag::warn_sme_locally_streaming_has_vl_args_returns)
<< /*IsArg=*/false;
if (llvm::any_of(FD->parameters(), [](ParmVarDecl *P) {
return P->getOriginalType()->isSizelessVectorType();
}))
Diag(FD->getLocation(),
diag::warn_sme_locally_streaming_has_vl_args_returns)
<< /*IsArg=*/true;
}
if (const auto *FPT = FD->getType()->getAs<FunctionProtoType>()) {
FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
UsesSM |= EPI.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask;
UsesZA |= FunctionType::getArmZAState(EPI.AArch64SMEAttributes) !=
FunctionType::ARM_None;
UsesZT0 |= FunctionType::getArmZT0State(EPI.AArch64SMEAttributes) !=
FunctionType::ARM_None;
}

ASTContext &Context = getASTContext();
if (UsesSM || UsesZA) {
llvm::StringMap<bool> FeatureMap;
Context.getFunctionFeatureMap(FeatureMap, FD);
if (!FeatureMap.contains("sme")) {
if (UsesSM)
Diag(FD->getLocation(),
diag::err_sme_definition_using_sm_in_non_sme_target);
else
Diag(FD->getLocation(),
diag::err_sme_definition_using_za_in_non_sme_target);
}
}
if (UsesZT0) {
llvm::StringMap<bool> FeatureMap;
Context.getFunctionFeatureMap(FeatureMap, FD);
if (!FeatureMap.contains("sme2")) {
Diag(FD->getLocation(),
diag::err_sme_definition_using_zt0_in_non_sme2_target);
}
}
}

} // namespace clang
55 changes: 3 additions & 52 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include "clang/Sema/ParsedTemplate.h"
#include "clang/Sema/Scope.h"
#include "clang/Sema/ScopeInfo.h"
#include "clang/Sema/SemaARM.h"
#include "clang/Sema/SemaCUDA.h"
#include "clang/Sema/SemaHLSL.h"
#include "clang/Sema/SemaInternal.h"
Expand Down Expand Up @@ -12286,58 +12287,8 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
}
}

// Check if the function definition uses any AArch64 SME features without
// having the '+sme' feature enabled and warn user if sme locally streaming
// function returns or uses arguments with VL-based types.
if (DeclIsDefn) {
const auto *Attr = NewFD->getAttr<ArmNewAttr>();
bool UsesSM = NewFD->hasAttr<ArmLocallyStreamingAttr>();
bool UsesZA = Attr && Attr->isNewZA();
bool UsesZT0 = Attr && Attr->isNewZT0();

if (NewFD->hasAttr<ArmLocallyStreamingAttr>()) {
if (NewFD->getReturnType()->isSizelessVectorType())
Diag(NewFD->getLocation(),
diag::warn_sme_locally_streaming_has_vl_args_returns)
<< /*IsArg=*/false;
if (llvm::any_of(NewFD->parameters(), [](ParmVarDecl *P) {
return P->getOriginalType()->isSizelessVectorType();
}))
Diag(NewFD->getLocation(),
diag::warn_sme_locally_streaming_has_vl_args_returns)
<< /*IsArg=*/true;
}
if (const auto *FPT = NewFD->getType()->getAs<FunctionProtoType>()) {
FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
UsesSM |=
EPI.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask;
UsesZA |= FunctionType::getArmZAState(EPI.AArch64SMEAttributes) !=
FunctionType::ARM_None;
UsesZT0 |= FunctionType::getArmZT0State(EPI.AArch64SMEAttributes) !=
FunctionType::ARM_None;
}

if (UsesSM || UsesZA) {
llvm::StringMap<bool> FeatureMap;
Context.getFunctionFeatureMap(FeatureMap, NewFD);
if (!FeatureMap.contains("sme")) {
if (UsesSM)
Diag(NewFD->getLocation(),
diag::err_sme_definition_using_sm_in_non_sme_target);
else
Diag(NewFD->getLocation(),
diag::err_sme_definition_using_za_in_non_sme_target);
}
}
if (UsesZT0) {
llvm::StringMap<bool> FeatureMap;
Context.getFunctionFeatureMap(FeatureMap, NewFD);
if (!FeatureMap.contains("sme2")) {
Diag(NewFD->getLocation(),
diag::err_sme_definition_using_zt0_in_non_sme2_target);
}
}
}
if (DeclIsDefn && Context.getTargetInfo().getTriple().isAArch64())
ARM().CheckSMEFunctionDefAttributes(NewFD);

return Redeclaration;
}
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Sema/SemaLambda.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "clang/Sema/Lookup.h"
#include "clang/Sema/Scope.h"
#include "clang/Sema/ScopeInfo.h"
#include "clang/Sema/SemaARM.h"
#include "clang/Sema/SemaCUDA.h"
#include "clang/Sema/SemaInternal.h"
#include "clang/Sema/SemaOpenMP.h"
Expand Down Expand Up @@ -1454,6 +1455,9 @@ void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
// Attributes on the lambda apply to the method.
ProcessDeclAttributes(CurScope, Method, ParamInfo);

if (Context.getTargetInfo().getTriple().isAArch64())
ARM().CheckSMEFunctionDefAttributes(Method);

// CUDA lambdas get implicit host and device attributes.
if (getLangOpts().CUDA)
CUDA().SetLambdaAttrs(Method);
Expand Down
8 changes: 7 additions & 1 deletion clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -std=c++23 -fsyntax-only -verify %s
Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Jan 6, 2025

Choose a reason for hiding this comment

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

What about this change requires c++23?

Copy link
Contributor Author

@kmclaughlin-arm kmclaughlin-arm Jan 7, 2025

Choose a reason for hiding this comment

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

I think adding attributes to lambdas before the parameter list was only allowed in C++23. If I don't pass this flag, the error emitted is instead "'attribute' in this position is a C++23 extension".


// This test is testing the diagnostics that Clang emits when compiling without '+sme'.

Expand Down Expand Up @@ -48,3 +48,9 @@ void streaming_compatible_def2(void (*streaming_fn_ptr)(void) __arm_streaming,
// Also test when call-site is not a function.
int streaming_decl_ret_int() __arm_streaming;
int x = streaming_decl_ret_int(); // expected-error {{call to a streaming function requires 'sme'}}

void sme_attrs_lambdas() {
[] __arm_locally_streaming () { return; }(); // expected-error {{function executed in streaming-SVE mode requires 'sme'}}
[] __arm_new("za") () { return; }(); // expected-error {{function using ZA state requires 'sme'}}
[] __arm_new("zt0") () { return; }(); // expected-error {{function using ZT0 state requires 'sme2'}}
}
Loading