[Clang] Demote always_inline error to warning for mismatching SME attrs #100740

sdesmalen-arm · 2024-07-26T13:31:43Z

PR #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).

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).

llvmbot · 2024-07-26T13:32:15Z

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang

Author: Sander de Smalen (sdesmalen-arm)

Changes

PR #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).

Full diff: https://github.com/llvm/llvm-project/pull/100740.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticFrontendKinds.td (+3)
(modified) clang/lib/CodeGen/Targets/AArch64.cpp (+4-2)
(modified) clang/test/CodeGen/aarch64-sme-inline-streaming-attrs.c (+3-3)

diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index 12a4617c64d87..04f87f5217067 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 result in different 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..2c565f6225468 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 result in different 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 result in different 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 result in different runtime behaviour}}
     inlined_fn_streaming_compatible();
     inlined_fn_streaming();
     inlined_fn_local();

Sirraide · 2024-07-26T13:43:04Z

At a glance, this seems like a good use case for a warning + DefaultError?

sdesmalen-arm · 2024-07-26T14:20:56Z

At a glance, this seems like a good use case for a warning + DefaultError?

I'd rather not make that change, because in practice it will be unlikely that inlining will result in changes to runtime behaviour. We'd rather not burden the users with explicit errors for something that may be fine, as it's more a limitation of Clang that it has to emit this message in the first place. Separate to that, it might not be directly clear to users that the error can be demoted into a warning.

Thanks for the suggestion though!

Sirraide · 2024-07-26T14:21:35Z

it's more a limitation of Clang that it has to emit this message in the first place

Fair enough, that makes sense

sdesmalen-arm · 2024-07-29T08:30:06Z

/cherry-pick 5430f73

…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)

llvmbot · 2024-07-29T08:34:58Z

/pull-request #100987

… SME attrs" (#100991) Reverts #100740

sdesmalen-arm requested review from SamTebbs33, jroelofs and paulwalker-arm July 26, 2024 13:31

llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Jul 26, 2024

Rephrase warning

377b19c

sdesmalen-arm force-pushed the sme-demote-always-inline-error branch from 4ddaab2 to 377b19c Compare July 26, 2024 15:33

paulwalker-arm approved these changes Jul 26, 2024

View reviewed changes

sdesmalen-arm added this to the LLVM 19.X Release milestone Jul 29, 2024

sdesmalen-arm merged commit 5430f73 into llvm:main Jul 29, 2024
7 checks passed

sdesmalen-arm mentioned this pull request Jul 29, 2024

Revert "[Clang] Demote always_inline error to warning for mismatching SME attrs" #100991

Merged

sdesmalen-arm added a commit that referenced this pull request Jul 29, 2024

Revert "[Clang] Demote always_inline error to warning for mismatching…

e3a3397

… SME attrs" (#100991) Reverts #100740

sdesmalen-arm mentioned this pull request Nov 20, 2024

[clang][SME] Ignore flatten/clang::always_inline statements for callees with mismatched streaming attributes #116391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Demote always_inline error to warning for mismatching SME attrs #100740

[Clang] Demote always_inline error to warning for mismatching SME attrs #100740

Uh oh!

sdesmalen-arm commented Jul 26, 2024

Uh oh!

llvmbot commented Jul 26, 2024 •

edited

Loading

Uh oh!

Sirraide commented Jul 26, 2024

Uh oh!

sdesmalen-arm commented Jul 26, 2024

Uh oh!

Sirraide commented Jul 26, 2024 •

edited

Loading

Uh oh!

Uh oh!

sdesmalen-arm commented Jul 29, 2024

Uh oh!

llvmbot commented Jul 29, 2024

Uh oh!

Uh oh!

[Clang] Demote always_inline error to warning for mismatching SME attrs #100740

[Clang] Demote always_inline error to warning for mismatching SME attrs #100740

Uh oh!

Conversation

sdesmalen-arm commented Jul 26, 2024

Uh oh!

llvmbot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Jul 26, 2024

Uh oh!

sdesmalen-arm commented Jul 26, 2024

Uh oh!

Sirraide commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sdesmalen-arm commented Jul 29, 2024

Uh oh!

llvmbot commented Jul 29, 2024

Uh oh!

Uh oh!

llvmbot commented Jul 26, 2024 •

edited

Loading

Sirraide commented Jul 26, 2024 •

edited

Loading