Skip to content

[Clang][SME] Detect always_inline used with mismatched streaming attributes #77936

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

Merged
merged 18 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticFrontendKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,10 @@ def err_builtin_needs_feature : Error<"%0 needs target feature %1">;
def err_function_needs_feature : Error<
"always_inline function %1 requires target feature '%2', but would "
"be inlined into function %0 that is compiled without support for '%2'">;
def err_function_always_inline_attribute_mismatch : Error<
"always_inline function %1 and its caller %0 have mismatching %2 attributes">;
def err_function_always_inline_new_za : Error<
"always_inline function %0 has new za state">;

def warn_avx_calling_convention
: Warning<"AVX vector %select{return|argument}0 of type %1 without '%2' "
Expand Down
43 changes: 43 additions & 0 deletions clang/lib/CodeGen/Targets/AArch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "ABIInfoImpl.h"
#include "TargetInfo.h"
#include "clang/Basic/DiagnosticFrontend.h"

using namespace clang;
using namespace clang::CodeGen;
Expand Down Expand Up @@ -155,6 +156,11 @@ class AArch64TargetCodeGenInfo : public TargetCodeGenInfo {
}
return TargetCodeGenInfo::isScalarizableAsmOperand(CGF, Ty);
}

void checkFunctionCallABI(CodeGenModule &CGM, SourceLocation CallLoc,
const FunctionDecl *Caller,
const FunctionDecl *Callee,
const CallArgList &Args) const override;
};

class WindowsAArch64TargetCodeGenInfo : public AArch64TargetCodeGenInfo {
Expand Down Expand Up @@ -814,6 +820,43 @@ Address AArch64ABIInfo::EmitMSVAArg(CodeGenFunction &CGF, Address VAListAddr,
/*allowHigherAlign*/ false);
}

static bool isStreaming(const FunctionDecl *F) {
if (F->hasAttr<ArmLocallyStreamingAttr>())
return true;
if (const auto *T = F->getType()->getAs<FunctionProtoType>())
return T->getAArch64SMEAttributes() & FunctionType::SME_PStateSMEnabledMask;
return false;
}

static bool isStreamingCompatible(const FunctionDecl *F) {
if (const auto *T = F->getType()->getAs<FunctionProtoType>())
return T->getAArch64SMEAttributes() &
FunctionType::SME_PStateSMCompatibleMask;
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@sdesmalen-arm WDYT about making these two free functions into members of FunctionDecl? Seems like copies of these helpers are proliferating.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes it makes sense to create a single interface for these and remove any duplicates!

I think we'll want to have two interfaces:

  • One for QualType for the type attributes (declared in AST/Type.h?)
  • One for FunctionDecl that also checks the declaration attributes (declared in AST/Decl.h?)

I'm happy for it to be done as part of this patch or as a follow-up to keep this patch simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good to me! I'd prefer to do it in a follow-up patch.


void AArch64TargetCodeGenInfo::checkFunctionCallABI(
CodeGenModule &CGM, SourceLocation CallLoc, const FunctionDecl *Caller,
const FunctionDecl *Callee, const CallArgList &Args) const {
if (!Caller || !Callee || !Callee->hasAttr<AlwaysInlineAttr>())
return;

bool CallerIsStreaming = isStreaming(Caller);
bool CalleeIsStreaming = isStreaming(Callee);
bool CallerIsStreamingCompatible = isStreamingCompatible(Caller);
bool CalleeIsStreamingCompatible = isStreamingCompatible(Callee);

if (!CalleeIsStreamingCompatible &&
(CallerIsStreaming != CalleeIsStreaming || CallerIsStreamingCompatible))
CGM.getDiags().Report(CallLoc,
diag::err_function_always_inline_attribute_mismatch)
<< Caller->getDeclName() << Callee->getDeclName() << "streaming";
if (auto *NewAttr = Callee->getAttr<ArmNewAttr>())
if (NewAttr->isNewZA())
CGM.getDiags().Report(CallLoc, diag::err_function_always_inline_new_za)
<< Callee->getDeclName();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is missing a check to see if the call requires the lazy-save mechanism as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, thanks!


std::unique_ptr<TargetCodeGenInfo>
CodeGen::createAArch64TargetCodeGenInfo(CodeGenModule &CGM,
AArch64ABIKind Kind) {
Expand Down
47 changes: 47 additions & 0 deletions clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -S -target-feature +sme -verify -DTEST_NONE %s
// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -S -target-feature +sme -verify -DTEST_COMPATIBLE %s
// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -S -target-feature +sme -verify -DTEST_STREAMING %s
// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -S -target-feature +sme -verify -DTEST_LOCALLY %s

#define __ai __attribute__((always_inline))
__ai void inlined_fn(void) {}
__ai void inlined_fn_streaming_compatible(void) __arm_streaming_compatible {}
__ai void inlined_fn_streaming(void) __arm_streaming {}
__ai __arm_locally_streaming void inlined_fn_local(void) {}

#ifdef TEST_NONE
void caller(void) {
inlined_fn();
inlined_fn_streaming_compatible();
inlined_fn_streaming(); // expected-error {{always_inline function 'inlined_fn_streaming' and its caller 'caller' have mismatching streaming attributes}}
inlined_fn_local(); // expected-error {{always_inline function 'inlined_fn_local' and its caller 'caller' have mismatching streaming attributes}}
}
#endif

#ifdef TEST_COMPATIBLE
void caller_compatible(void) __arm_streaming_compatible {
inlined_fn(); // expected-error {{always_inline function 'inlined_fn' and its caller 'caller_compatible' have mismatching streaming attributes}}
inlined_fn_streaming_compatible();
inlined_fn_streaming(); // expected-error {{always_inline function 'inlined_fn_streaming' and its caller 'caller_compatible' have mismatching streaming attributes}}
inlined_fn_local(); // expected-error {{always_inline function 'inlined_fn_local' and its caller 'caller_compatible' have mismatching streaming attributes}}
}
#endif

#ifdef TEST_STREAMING
void caller_streaming(void) __arm_streaming {
inlined_fn(); // expected-error {{always_inline function 'inlined_fn' and its caller 'caller_streaming' have mismatching streaming attributes}}
inlined_fn_streaming_compatible();
inlined_fn_streaming();
inlined_fn_local();
}
#endif

#ifdef TEST_LOCALLY
__arm_locally_streaming
void caller_local(void) {
inlined_fn(); // expected-error {{always_inline function 'inlined_fn' and its caller 'caller_local' have mismatching streaming attributes}}
inlined_fn_streaming_compatible();
inlined_fn_streaming();
inlined_fn_local();
}
#endif