[clang][sema][FMV] Forbid multi-versioning arm_streaming functions. #81268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The streaming mode change is incompatible with the ifunc mechanism used to implement FMV: we can't conditionally change it based on the particular callee that is resolved at runtime. Fixes: llvm#80077
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Jon Roelofs (jroelofs) ChangesThe streaming mode change is incompatible with the ifunc mechanism used to implement FMV: we can't conditionally change it based on the particular callee that is resolved at runtime. Fixes: #80077 Full diff: https://github.com/llvm/llvm-project/pull/81268.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b4dc4feee8e63a..83b89d1449f420 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3728,6 +3728,8 @@ def err_sme_definition_using_zt0_in_non_sme2_target : Error<
"function using ZT0 state requires 'sme2'">;
def err_conflicting_attributes_arm_state : Error<
"conflicting attributes for state '%0'">;
+def err_sme_streaming_cannot_be_multiversioned : Error<
+ "streaming function cannot be multi-versioned">;
def err_unknown_arm_state : Error<
"unknown state '%0'">;
def err_missing_arm_state : Error<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 3c26003b5bda7f..9930ca2eb370e5 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4838,13 +4838,12 @@ class Sema final {
llvm::Error isValidSectionSpecifier(StringRef Str);
bool checkSectionName(SourceLocation LiteralLoc, StringRef Str);
bool checkTargetAttr(SourceLocation LiteralLoc, StringRef Str);
- bool checkTargetVersionAttr(SourceLocation LiteralLoc, StringRef &Str,
- bool &isDefault);
- bool
- checkTargetClonesAttrString(SourceLocation LiteralLoc, StringRef Str,
- const StringLiteral *Literal, bool &HasDefault,
- bool &HasCommas, bool &HasNotDefault,
- SmallVectorImpl<SmallString<64>> &StringsBuffer);
+ bool checkTargetVersionAttr(SourceLocation LiteralLoc, Decl *D,
+ StringRef &Str, bool &isDefault);
+ bool checkTargetClonesAttrString(
+ SourceLocation LiteralLoc, StringRef Str, const StringLiteral *Literal,
+ Decl *D, bool &HasDefault, bool &HasCommas, bool &HasNotDefault,
+ SmallVectorImpl<SmallString<64>> &StringsBuffer);
bool checkMSInheritanceAttrOnDefinition(
CXXRecordDecl *RD, SourceRange Range, bool BestCase,
MSInheritanceModel SemanticSpelling);
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index d785714c4d811e..2a7c11cd4351f4 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3501,9 +3501,18 @@ bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
return false;
}
+static bool isArmStreaming(const FunctionDecl *FD) {
+ if (FD->hasAttr<ArmLocallyStreamingAttr>())
+ return true;
+ if (const auto *T = FD->getType()->getAs<FunctionProtoType>())
+ if (T->getAArch64SMEAttributes() & FunctionType::SME_PStateSMEnabledMask)
+ return true;
+ return false;
+}
+
// Check Target Version attrs
-bool Sema::checkTargetVersionAttr(SourceLocation LiteralLoc, StringRef &AttrStr,
- bool &isDefault) {
+bool Sema::checkTargetVersionAttr(SourceLocation LiteralLoc, Decl *D,
+ StringRef &AttrStr, bool &isDefault) {
enum FirstParam { Unsupported };
enum SecondParam { None };
enum ThirdParam { Target, TargetClones, TargetVersion };
@@ -3519,6 +3528,8 @@ bool Sema::checkTargetVersionAttr(SourceLocation LiteralLoc, StringRef &AttrStr,
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << CurFeature << TargetVersion;
}
+ if (isArmStreaming(cast<FunctionDecl>(D)))
+ return Diag(LiteralLoc, diag::err_sme_streaming_cannot_be_multiversioned);
return false;
}
@@ -3527,7 +3538,7 @@ static void handleTargetVersionAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
SourceLocation LiteralLoc;
bool isDefault = false;
if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &LiteralLoc) ||
- S.checkTargetVersionAttr(LiteralLoc, Str, isDefault))
+ S.checkTargetVersionAttr(LiteralLoc, D, Str, isDefault))
return;
// Do not create default only target_version attribute
if (!isDefault) {
@@ -3550,7 +3561,7 @@ static void handleTargetAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
bool Sema::checkTargetClonesAttrString(
SourceLocation LiteralLoc, StringRef Str, const StringLiteral *Literal,
- bool &HasDefault, bool &HasCommas, bool &HasNotDefault,
+ Decl *D, bool &HasDefault, bool &HasCommas, bool &HasNotDefault,
SmallVectorImpl<SmallString<64>> &StringsBuffer) {
enum FirstParam { Unsupported, Duplicate, Unknown };
enum SecondParam { None, CPU, Tune };
@@ -3619,6 +3630,9 @@ bool Sema::checkTargetClonesAttrString(
HasNotDefault = true;
}
}
+ if (isArmStreaming(cast<FunctionDecl>(D)))
+ return Diag(LiteralLoc,
+ diag::err_sme_streaming_cannot_be_multiversioned);
} else {
// Other targets ( currently X86 )
if (Cur.starts_with("arch=")) {
@@ -3670,7 +3684,7 @@ static void handleTargetClonesAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
if (!S.checkStringLiteralArgumentAttr(AL, I, CurStr, &LiteralLoc) ||
S.checkTargetClonesAttrString(
LiteralLoc, CurStr,
- cast<StringLiteral>(AL.getArgAsExpr(I)->IgnoreParenCasts()),
+ cast<StringLiteral>(AL.getArgAsExpr(I)->IgnoreParenCasts()), D,
HasDefault, HasCommas, HasNotDefault, StringsBuffer))
return;
}
diff --git a/clang/test/Sema/aarch64-sme-func-attrs.c b/clang/test/Sema/aarch64-sme-func-attrs.c
index 2bf1886951f1f7..2f8c3eb6aed9d8 100644
--- a/clang/test/Sema/aarch64-sme-func-attrs.c
+++ b/clang/test/Sema/aarch64-sme-func-attrs.c
@@ -454,3 +454,34 @@ void unimplemented_spill_fill_za(void (*share_zt0_only)(void) __arm_inout("zt0")
// expected-note@+1 {{add '__arm_preserves("za")' to the callee if it preserves ZA}}
share_zt0_only();
}
+
+// expected-cpp-error@+2 {{streaming function cannot be multi-versioned}}
+// expected-error@+1 {{streaming function cannot be multi-versioned}}
+__attribute__((target_version("sme2")))
+ // expected-cpp-note@+2 {{previous declaration is here}}
+ // expected-note@+1 {{previous declaration is here}}
+void cannot_work_version(void) __arm_streaming {}
+
+// expected-cpp-error@+3 {{function declared 'void ()' was previously declared 'void () __arm_streaming', which has different SME function attributes}}
+// expected-error@+2 {{function declared 'void (void)' was previously declared 'void (void) __arm_streaming', which has different SME function attributes}}
+__attribute__((target_version("default")))
+void cannot_work_version(void) {}
+
+// expected-cpp-error@+2 {{streaming function cannot be multi-versioned}}
+// expected-error@+1 {{streaming function cannot be multi-versioned}}
+__attribute__((target_clones("sme2")))
+void cannot_work_clones(void) __arm_streaming {}
+
+__attribute__((target("sme2")))
+void just_fine_sme_streaming(void) __arm_streaming {}
+__attribute__((target_version("sme2")))
+void just_fine(void) { just_fine_sme_streaming(); }
+__attribute__((target_version("default")))
+void just_fine(void) { }
+
+
+void fmv_caller() {
+ cannot_work_version();
+ cannot_work_clones();
+ just_fine();
+}
|
Comment on lines
461
to
462
| // expected-cpp-note@+2 {{previous declaration is here}} | ||
| // expected-note@+1 {{previous declaration is here}} |
There was a problem hiding this comment.
nit: I guess these are notes corresponding to the errors on line 465 and 466 right? If so, could you move them to those lines? (and reference it with a negative line offset)
clang/lib/Sema/SemaDeclAttr.cpp
Outdated
Comment on lines
3505
to
3506
| if (FD->hasAttr<ArmLocallyStreamingAttr>()) | ||
| return true; |
There was a problem hiding this comment.
Is this check required? I thought it was more about the interface that can't be both "streaming" and "non-streaming" (for different versions of the function), because the caller needs to know in which mode to call the function. However, the callee is free to change streaming mode in its implementation.
There was a problem hiding this comment.
Oh, you're right, we don't need this one, since the ABI isn't different from a normal function: https://arm-software.github.io/acle/main/acle.html#changing-streaming-mode-locally
sdesmalen-arm
approved these changes
Feb 12, 2024
clang/lib/Sema/SemaDeclAttr.cpp
Outdated
| @@ -3501,9 +3501,16 @@ bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) { | |||
| return false; | |||
| } | |||
|
|
|||
| static bool hasStreamingModeChangeInABI(const FunctionDecl *FD) { | |||
There was a problem hiding this comment.
nit:
Suggested change
| static bool hasStreamingModeChangeInABI(const FunctionDecl *FD) { | |
| static bool hasArmStreamingInterface(const FunctionDecl *FD) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The streaming mode change is incompatible with the ifunc mechanism used to implement FMV: we can't conditionally change it based on the particular callee that is resolved at runtime.
Fixes: #80077