release/19.x: [Clang] Demote always_inline error to warning for mismatching SME attrs (#100740) #100987
Conversation
This file contains hidden or 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
…rs (llvm#100740) PR llvm#77936 introduced a diagnostic to avoid calls being inlined into functions with a different streaming mode, because inlining those functions may result in different runtime behaviour. This was necessary because LLVM doesn't check whether inlining is possible and thus blindly inlines the function without checking feasibility. In practice however, this introduces an artificial restriction that the user may not be able to work around. Calling an `always_inline` function from some header file that is out of the control of the user would result in an error that the user cannot remedy. Therefore, this patch demotes the error into a warning (for calls from streaming[-compatible] -> non-streaming), but the proper fix would be to fix the AlwaysInliner in LLVM to avoid inlining when it has analyzed the callee and has determined that inlining is not possible. Calling an always_inline function for calls from non-streaming -> streaming will remain an error, because there is little pre-existing code for SME, so it is expected that the header file can be modified by the user (e.g. by using `__arm_streaming_compatible` if the code is claimed to be compatible). (cherry picked from commit 5430f73)
|
@paulwalker-arm What do you think about merging this PR to the release branch? |
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-clang Author: None (llvmbot) ChangesBackport 5430f73 Requested by: @sdesmalen-arm Full diff: https://github.com/llvm/llvm-project/pull/100987.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 12a4617c64d87..8a1462c670d68 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -288,6 +288,9 @@ def err_function_needs_feature : Error<
let CategoryName = "Codegen ABI Check" in {
def err_function_always_inline_attribute_mismatch : Error<
"always_inline function %1 and its caller %0 have mismatching %2 attributes">;
+def warn_function_always_inline_attribute_mismatch : Warning<
+ "always_inline function %1 and its caller %0 have mismatching %2 attributes, "
+ "inlining may change runtime behaviour">, InGroup<AArch64SMEAttributes>;
def err_function_always_inline_new_za : Error<
"always_inline function %0 has new za state">;
diff --git a/clang/lib/CodeGen/Targets/AArch64.cpp b/clang/lib/CodeGen/Targets/AArch64.cpp
index b9df54b0c67c4..1dec3cd40ebd2 100644
--- a/clang/lib/CodeGen/Targets/AArch64.cpp
+++ b/clang/lib/CodeGen/Targets/AArch64.cpp
@@ -883,8 +883,10 @@ void AArch64TargetCodeGenInfo::checkFunctionCallABIStreaming(
if (!CalleeIsStreamingCompatible &&
(CallerIsStreaming != CalleeIsStreaming || CallerIsStreamingCompatible))
- CGM.getDiags().Report(CallLoc,
- diag::err_function_always_inline_attribute_mismatch)
+ CGM.getDiags().Report(
+ CallLoc, CalleeIsStreaming
+ ? diag::err_function_always_inline_attribute_mismatch
+ : diag::warn_function_always_inline_attribute_mismatch)
<< Caller->getDeclName() << Callee->getDeclName() << "streaming";
if (auto *NewAttr = Callee->getAttr<ArmNewAttr>())
if (NewAttr->isNewZA())
diff --git a/clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c b/clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c
index 25aebeced9379..98370a888ac6c 100644
--- a/clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c
+++ b/clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c
@@ -20,7 +20,7 @@ void caller(void) {
#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(); // expected-warning {{always_inline function 'inlined_fn' and its caller 'caller_compatible' have mismatching streaming attributes, inlining may change runtime behaviour}}
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}}
@@ -29,7 +29,7 @@ void caller_compatible(void) __arm_streaming_compatible {
#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(); // expected-warning {{always_inline function 'inlined_fn' and its caller 'caller_streaming' have mismatching streaming attributes, inlining may change runtime behaviour}}
inlined_fn_streaming_compatible();
inlined_fn_streaming();
inlined_fn_local();
@@ -39,7 +39,7 @@ void caller_streaming(void) __arm_streaming {
#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(); // expected-warning {{always_inline function 'inlined_fn' and its caller 'caller_local' have mismatching streaming attributes, inlining may change runtime behaviour}}
inlined_fn_streaming_compatible();
inlined_fn_streaming();
inlined_fn_local();
|
sdesmalen-arm
requested changes
Jul 29, 2024
There was a problem hiding this comment.
For some odd reason, clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c seems to be failing on some buildbots with an error that says:
unable to create target: No available targets are compatible with triple "aarch64-none-linux-gnu"'. I suspect because the test is missing aREQUIRES: aarch64-registered-target, but I'm puzzled why that test didn't fail before, because my patch doesn't introduce this test and doesn't change the RUN lines, all this patch does is change one of the diagnostic messages.
In any case, I seemed to have jumped the gun creating this cherry-pick in the first place, I thought the change was trivial enough to do so especially after testing locally. My apologies for the noise.
I'll revert the patch and fix the issue, and will then create another cherry-pick.
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.
Backport 5430f73
Requested by: @sdesmalen-arm