[Clang][AArch64] Warn when calling streaming/non-streaming about vect… #79842

dtemirbulatov · 2024-01-29T14:50:04Z

…or size might be different.

The compiler doesn't know in advance if the streaming and non-streaming vector-lengths are different, so it should be safe to give a warning diagnostic to warn the user about possible undefined behaviour. If the user knows the vector lengths are equal, they can disable the warning separately.

llvmbot · 2024-01-29T14:50:39Z

@llvm/pr-subscribers-clang

Author: Dinar Temirbulatov (dtemirbulatov)

Changes

…or size might be different.

The compiler doesn't know in advance if the streaming and non-streaming vector-lengths are different, so it should be safe to give a warning diagnostic to warn the user about possible undefined behaviour. If the user knows the vector lengths are equal, they can disable the warning separately.

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

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+24)
(modified) clang/lib/Sema/SemaChecking.cpp (+42)
(modified) clang/test/Sema/aarch64-sme-func-attrs.c (+66-2)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 24d32cb87c89e24..37fea5746936c7a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3717,6 +3717,30 @@ def err_sme_definition_using_za_in_non_sme_target : Error<
   "function using ZA state requires 'sme'">;
 def err_sme_definition_using_zt0_in_non_sme2_target : Error<
   "function using ZT0 state requires 'sme2'">;
+def warn_sme_streaming_caller_pass_args_to_non_streaming : Warning<
+  "streaming caller passes a VL-dependent argument to non-streaming callee, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;
+def warn_sme_non_streaming_callee_returns_to_streaming : Warning<
+  "non-streaming callee returns a VL-dependent value to streaming caller, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;
+def warn_sme_non_streaming_caller_pass_args_to_streaming : Warning<
+  "non-streaming caller passes a VL-dependent argument to streaming callee, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;
+def warn_sme_non_streaming_caller_returns_to_streaming : Warning<
+  "non-streaming callee returns a VL-dependent value to streaming caller, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;
+def warn_sme_locally_streaming_has_vl_args : Warning<
+  "non-streaming callee receives a VL-dependent argument and the callee has an arm_locally_streaming attribute, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;
+def warn_sme_locally_streaming_returns_vl : Warning<
+  "non-streaming callee returns a VL-dependent value and the callee has an arm_locally_streaming attribute, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;
 def err_conflicting_attributes_arm_state : Error<
   "conflicting attributes for state '%0'">;
 def err_unknown_arm_state : Error<
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 502b24bcdf8b42b..e668a45c69e5f91 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -7480,6 +7480,7 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
     // For variadic functions, we may have more args than parameters.
     // For some K&R functions, we may have less args than parameters.
     const auto N = std::min<unsigned>(Proto->getNumParams(), Args.size());
+    bool AnyScalableArgs = false;
     for (unsigned ArgIdx = 0; ArgIdx < N; ++ArgIdx) {
       // Args[ArgIdx] can be null in malformed code.
       if (const Expr *Arg = Args[ArgIdx]) {
@@ -7493,6 +7494,8 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
           checkAIXMemberAlignment((Arg->getExprLoc()), Arg);
 
         QualType ParamTy = Proto->getParamType(ArgIdx);
+        if (ParamTy->isSizelessVectorType())
+          AnyScalableArgs = true;
         QualType ArgTy = Arg->getType();
         CheckArgAlignment(Arg->getExprLoc(), FDecl, std::to_string(ArgIdx + 1),
                           ArgTy, ParamTy);
@@ -7513,6 +7516,45 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
       }
     }
 
+    auto *CallerFD = dyn_cast<FunctionDecl>(CurContext);
+    if (FD && CallerFD && Context.getTargetInfo().hasFeature("sme") &&
+        !FD->getBuiltinID()) {
+      // If the callee has an AArch64 SME __arm_locally_streaming attribute
+      // warn if this function returns VL-based value or pass any such argument,
+      // the streaming and non-streaming vector lengths may be different.
+      ArmStreamingType CalleeFnType = getArmStreamingFnType(FD);
+      ArmStreamingType CallerFnType = getArmStreamingFnType(CallerFD);
+      if (FD->hasAttr<ArmLocallyStreamingAttr>() &&
+          CallerFnType != ArmStreaming) {
+        if (AnyScalableArgs)
+          Diag(Loc, diag::warn_sme_locally_streaming_has_vl_args);
+        if (FD->getReturnType()->isSizelessVectorType())
+          Diag(Loc, diag::warn_sme_locally_streaming_returns_vl);
+      }
+      // If the caller is a non-streaming function and the callee has a
+      // streaming attribute. If it passed any VL-based arguments or return
+      // VL-based value, then warn that the streaming and non-streaming vector
+      // lengths may be different.
+      if (CallerFnType != ArmStreaming) {
+        if (CalleeFnType == ArmStreaming) {
+          if (AnyScalableArgs)
+            Diag(Loc,
+                 diag::warn_sme_non_streaming_caller_pass_args_to_streaming);
+          if (FD->getReturnType()->isSizelessVectorType())
+            Diag(Loc, diag::warn_sme_non_streaming_caller_returns_to_streaming);
+        }
+      } else if (!FD->hasAttr<ArmLocallyStreamingAttr>()) {
+        // If the callee is a non-streaming function and the caller has
+        // streaming attribute. If it passed any VL-based arguments or return
+        // VL-based value, then warn that the streaming and non-streaming vector
+        // lengths may be different.
+        if (AnyScalableArgs)
+          Diag(Loc, diag::warn_sme_streaming_caller_pass_args_to_non_streaming);
+        if (FD->getReturnType()->isSizelessVectorType())
+          Diag(Loc, diag::warn_sme_non_streaming_callee_returns_to_streaming);
+      }
+    }
+
     // If the callee uses AArch64 SME ZA state but the caller doesn't define
     // any, then this is an error.
     FunctionType::ArmStateValue ArmZAState =
diff --git a/clang/test/Sema/aarch64-sme-func-attrs.c b/clang/test/Sema/aarch64-sme-func-attrs.c
index 97409ae7d6040c0..0a8e6e03a94f291 100644
--- a/clang/test/Sema/aarch64-sme-func-attrs.c
+++ b/clang/test/Sema/aarch64-sme-func-attrs.c
@@ -1,5 +1,6 @@
-// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -fsyntax-only -verify=expected-cpp -x c++ %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify=expected-cpp -x c++ %s
+#include <arm_sme.h>
 
 // Valid attributes
 
@@ -48,6 +49,9 @@ typedef void (*fptrty6) (void);
 fptrty6 cast_nza_func_to_normal() { return sme_arm_new_za; }
 fptrty6 cast_ls_func_to_normal() { return sme_arm_locally_streaming; }
 
+void sme_arm_streaming_with_vl_args(void) __arm_streaming;
+
+
 // Invalid attributes
 
 // expected-cpp-error@+4 {{'__arm_streaming_compatible' and '__arm_streaming' are not compatible}}
@@ -445,3 +449,63 @@ void conflicting_state_attrs_preserves_out_zt0(void) __arm_preserves("zt0") __ar
 // expected-cpp-error@+2 {{conflicting attributes for state 'zt0'}}
 // expected-error@+1 {{conflicting attributes for state 'zt0'}}
 void conflicting_state_attrs_preserves_inout_zt0(void) __arm_preserves("zt0") __arm_inout("zt0");
+
+void sme_streaming_with_vl_arg(svint32x4_t a) __arm_streaming { }
+
+svint32x4_t sme_streaming_returns_vl(void) __arm_streaming { svint32x4_t r; return r; }
+
+void sme_none_streaming_with_vl_arg(svint32x4_t a) { }
+
+svint32x4_t sme_none_streaming_returns_vl(void) { svint32x4_t r; return r; }
+
+__arm_locally_streaming void sme_locally_streaming_with_vl_arg(svint32x4_t a) { }
+
+__arm_locally_streaming svint32x4_t sme_locally_streaming_returns_vl(void) { svint32x4_t r; return r; }
+
+void sme_none_streaming_calling_streaming_with_vl_args() {
+  svint32x4_t a;
+  // expected-warning@+2 {{non-streaming caller passes a VL-dependent argument to streaming callee, the streaming and non-streaming vector lengths may be different}}
+  // expected-cpp-warning@+1 {{non-streaming caller passes a VL-dependent argument to streaming callee, the streaming and non-streaming vector lengths may be different}}
+  sme_streaming_with_vl_arg(a);
+}
+
+void sme_none_streaming_calling_streaming_with_return_vl() {
+  // expected-warning@+2 {{non-streaming callee returns a VL-dependent value to streaming caller, the streaming and non-streaming vector lengths may be different}}
+  // expected-cpp-warning@+1 {{non-streaming callee returns a VL-dependent value to streaming caller, the streaming and non-streaming vector lengths may be different}}
+  svint32x4_t r = sme_streaming_returns_vl();
+}
+
+void sme_streaming_calling_non_streaming_with_vl_args(void) __arm_streaming {
+  svint32x4_t a;
+  // expected-warning@+2 {{streaming caller passes a VL-dependent argument to non-streaming callee, the streaming and non-streaming vector lengths may be different}}
+  // expected-cpp-warning@+1 {{streaming caller passes a VL-dependent argument to non-streaming callee, the streaming and non-streaming vector lengths may be different}}
+  sme_none_streaming_with_vl_arg(a);
+}
+
+void sme_streaming_calling_non_streaming_with_return_vl(void) __arm_streaming {
+  // expected-warning@+2 {{non-streaming callee returns a VL-dependent value to streaming caller, the streaming and non-streaming vector lengths may be different}}
+  // expected-cpp-warning@+1 {{non-streaming callee returns a VL-dependent value to streaming caller, the streaming and non-streaming vector lengths may be different}}
+  svint32x4_t r = sme_streaming_returns_vl();
+}
+
+void sme_streaming_calling_locally_streaming_with_vl_args(void) __arm_streaming {
+  svint32x4_t a;
+  // expected-1warning@+2 {{non-streaming callee receives a VL-dependent argument and the callee has an arm_locally_streaming attribute, the streaming and non-streaming vector lengths may be different}}
+  // expected-1cpp-warning@+1 {{non-streaming callee receives a VL-dependent argument and the callee has an arm_locally_streaming attribute, the streaming and non-streaming vector lengths may be different}}
+  sme_locally_streaming_with_vl_arg(a);
+}
+
+void sme_streaming_calling_locally_streaming_with_return_vl(void) __arm_streaming {
+  // expected-1warning@+2 {{non-streaming callee returns a VL-dependent value and the callee has an arm_locally_streaming attribute, the streaming and non-streaming vector lengths may be different}}
+  // expected-1cpp-warning@+1 {{non-streaming callee returns a VL-dependent value and the callee has an arm_locally_streaming attribute, the streaming and non-streaming vector lengths may be different}}
+  svint32x4_t r = sme_locally_streaming_returns_vl();
+}
+
+void sme_none_streaming_calling_locally_streaming_with_vl_args(void) __arm_streaming {
+  svint32x4_t a;
+  sme_locally_streaming_with_vl_arg(a);
+}
+
+void sme_none_streaming_calling_locally_streaming_with_return_vl(void) __arm_streaming {
+  svint32x4_t r = sme_locally_streaming_returns_vl();
+}

sdesmalen-arm · 2024-02-02T13:37:03Z

clang/lib/Sema/SemaChecking.cpp

+      if (FD->hasAttr<ArmLocallyStreamingAttr>()) {
+        if (AnyScalableArgs)
+          Diag(Loc, diag::warn_sme_locally_streaming_has_vl_args);
+        if (FD->getReturnType()->isSizelessVectorType())
+          Diag(Loc, diag::warn_sme_locally_streaming_returns_vl);
+      }


__arm_locally_streaming is not a property of a function interface and therefore unrelated to a function call. That's why this code should live somewhere else.

sdesmalen-arm · 2024-02-02T13:41:26Z

clang/lib/Sema/SemaChecking.cpp

+          if (FD->getReturnType()->isSizelessVectorType())
+            Diag(Loc, diag::warn_sme_non_streaming_caller_returns_to_streaming);
+        }
+      } else if (!FD->hasAttr<ArmLocallyStreamingAttr>()) {


This is not correct. I get a diagnostic for this case:

svint8_t bar1(void) __arm_streaming; svint8_t foo1() __arm_streaming { return bar1(); // incorrectly gives a diagnostic }

sdesmalen-arm · 2024-02-02T13:42:30Z

clang/test/Sema/aarch64-sme-func-attrs.c

-// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -fsyntax-only -verify=expected-cpp -x c++ %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify=expected-cpp -x c++ %s
+#include <arm_sme.h>


Can you write this test without including arm_sme.h ?

ok, but then I have to define __clang_svint32x4_t.

sdesmalen-arm · 2024-02-02T13:49:03Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+def warn_sme_streaming_caller_pass_args_to_non_streaming : Warning<
+  "streaming caller passes a VL-dependent argument to non-streaming callee, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<IgnoredAttributes>;


I don't think 'IgnoredAttributes' is correct.

These diagnostics probably deserve to be in their own group.

But why not, There is warn_riscv_repeated_interrupt_attribute also uses IgnoredAttributes.

Because the attribute is not ignored.

Add new warning group AArch64SMEAttributes.

sdesmalen-arm · 2024-02-09T09:56:41Z

clang/lib/Sema/SemaChecking.cpp

@@ -7513,6 +7516,38 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
      }
    }

+    auto *CallerFD = dyn_cast<FunctionDecl>(CurContext);
+    if (FD && CallerFD && Context.getTargetInfo().hasFeature("sme") &&


This code requires that the function definition is available (if (FD && ...)), but the streaming mode is exposed through the interface, meaning that we shouldn't need FD in order to emit a diagnostic.

For example:

// I would expect a warning here. void test_n_to_s( __SVInt32_t arg, void (*sc)(__SVInt32_t arg) __arm_streaming) { sc(arg); }

sdesmalen-arm · 2024-02-09T09:58:51Z

clang/lib/Sema/SemaChecking.cpp

+      if (CallerFnType != ArmStreaming) {
+        if (CalleeFnType == ArmStreaming) {
+          if (AnyScalableArgs)
+            Diag(Loc,
+                 diag::warn_sme_non_streaming_caller_pass_args_to_streaming);
+          if (FD->getReturnType()->isSizelessVectorType())
+            Diag(Loc, diag::warn_sme_non_streaming_caller_returns_to_streaming);
+        }
+      } else if (CalleeFnType != ArmStreaming) {


This logic is not correct, because the following example should not result in a warning.

void sc_f(sv_ty arg) __arm_streaming_compatible; void test_s_to_sc2(sv_ty arg) __arm_streaming { sc_f(arg); }```

sdesmalen-arm · 2024-02-09T10:19:06Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+def warn_sme_streaming_caller_pass_args_to_non_streaming : Warning<
+  "streaming caller passes a VL-dependent argument to non-streaming callee, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<AArch64SMEAttributes>;
+def warn_sme_non_streaming_callee_returns_to_streaming : Warning<
+  "non-streaming callee returns a VL-dependent value to streaming caller, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<AArch64SMEAttributes>;
+def warn_sme_non_streaming_caller_pass_args_to_streaming : Warning<
+  "non-streaming caller passes a VL-dependent argument to streaming callee, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<AArch64SMEAttributes>;
+def warn_sme_non_streaming_caller_returns_to_streaming : Warning<
+  "non-streaming callee returns a VL-dependent value to streaming caller, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<AArch64SMEAttributes>;


nit: I think we can use a single message for this, e.g. "passing a VL-dependent argument to/from a function that has a different streaming-mode, is undefined behaviour"

sdesmalen-arm · 2024-02-09T10:19:31Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+def warn_sme_locally_streaming_has_vl_args : Warning<
+  "non-streaming callee receives a VL-dependent argument and the callee has an arm_locally_streaming attribute, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<AArch64SMEAttributes>;
+def warn_sme_locally_streaming_returns_vl : Warning<
+  "non-streaming callee returns a VL-dependent value and the callee has an arm_locally_streaming attribute, "
+  "the streaming and non-streaming vector lengths may be different">,
+  InGroup<AArch64SMEAttributes>;


These two can become a single message as well.

clang/test/Sema/aarch64-sme-func-attrs.c

sdesmalen-arm · 2024-02-14T08:19:09Z

clang/lib/Sema/SemaChecking.cpp

      if (CallerFnType != ArmStreaming) {
-        if (CalleeFnType == ArmStreaming) {
+        if (IsCalleeStreaming) {
          if (AnyScalableArgs)
-            Diag(Loc,
-                 diag::warn_sme_non_streaming_caller_pass_args_to_streaming);
-          if (FD->getReturnType()->isSizelessVectorType())
-            Diag(Loc, diag::warn_sme_non_streaming_caller_returns_to_streaming);
+            Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);
+          if (Proto->getReturnType()->isSizelessVectorType())
+            Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);


Do you think this could be simplified further?

sdesmalen-arm · 2024-02-14T08:22:40Z

clang/lib/Sema/SemaDecl.cpp

@@ -12239,7 +12239,8 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,

    if (UsesSM) {


The comment on line 12232-12233 needs an update.

sdesmalen-arm · 2024-02-14T08:25:03Z

clang/lib/Sema/SemaDecl.cpp

+      if (NewFD->getReturnType()->isSizelessVectorType())
+        Diag(NewFD->getLocation(),
+             diag::warn_sme_locally_streaming_has_vl_args_returns);
+      auto *FPT = NewFD->getType()->castAs<FunctionProtoType>();
+      bool AnyScalableArgs = false;
+      for (QualType T : FPT->param_types()) {
+        if (T->isSizelessVectorType()) {
+          AnyScalableArgs = true;
+          break;
+        }
+      }
+      if (AnyScalableArgs)
+        Diag(NewFD->getLocation(),
+             diag::warn_sme_locally_streaming_has_vl_args_returns);


Can this be written like something along the lines of:

if (NewFD->getReturnType()->isSizelessVectorType() || llvm::any_of(FPT->param_types(), [](QualType T) { return T->isSizelessVectorType(); })) Diag(....);

?

sdesmalen-arm · 2024-02-14T08:27:13Z

clang/lib/Sema/SemaDecl.cpp

+      if (NewFD->getReturnType()->isSizelessVectorType())
+        Diag(NewFD->getLocation(),
+             diag::warn_sme_locally_streaming_has_vl_args_returns);
+      auto *FPT = NewFD->getType()->castAs<FunctionProtoType>();


Is FunctionPrototype correct here? That means the check does not happen on unprototyped functions, such as __arm_locally_streaming void foo() { ... } in C (not C++).

clang/test/Sema/aarch64-sme-func-attrs.c

sdesmalen-arm · 2024-02-14T08:31:21Z

clang/lib/Sema/SemaChecking.cpp

+          Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);
+        if (Proto->getReturnType()->isSizelessVectorType())
+          Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);
+      }


Does this code support the following case:

void test_sc_to_n(sv_ty arg) __arm_streaming_compatible { n(arg); } // expect a diagnostic

?

sdesmalen-arm · 2024-02-14T08:31:55Z

clang/lib/Sema/SemaChecking.cpp

+                              (ExtInfo.AArch64SMEAttributes &
+                               FunctionType::SME_PStateSMCompatibleMask));


Streaming-compatible and streaming are not the same thing. i.e. if the callee is 'streaming-compatible', IsCalleeStreaming should be false.

sdesmalen-arm · 2024-02-14T08:35:16Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3717,6 +3717,14 @@ def err_sme_definition_using_za_in_non_sme_target : Error<
  "function using ZA state requires 'sme'">;
 def err_sme_definition_using_zt0_in_non_sme2_target : Error<
  "function using ZT0 state requires 'sme2'">;
+def warn_sme_streaming_pass_return_vl_to_non_streaming : Warning<
+  "passing a VL-dependent argument to/from a function that has a different"
+  " streaming-mode, is undefined behaviour">,


it is only undefined behaviour if the streaming VL != non-streaming VL.

sdesmalen-arm · 2024-02-29T14:02:53Z

clang/lib/Sema/SemaChecking.cpp

+    // non-compitable streaming attribute. If it passed any VL-based arguments
+    // or return VL-based value, then warn that the streaming and non-streaming
+    // vector lengths may be different.
+    if (CallerFD && Context.getTargetInfo().hasFeature("sme") && !IsBuiltin) {


Suggested change

if (CallerFD && Context.getTargetInfo().hasFeature("sme") && !IsBuiltin) {

if (CallerFD && !IsBuiltin) {

It is valid to compile the following function without +sme:

void streaming_compatible_fn(svint32_t v, void (*foo)(svint32_t)) __arm_streaming_compatible { foo(v); }

Done.

It is not.

sdesmalen-arm · 2024-02-29T14:03:22Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3717,6 +3717,16 @@ def err_sme_definition_using_za_in_non_sme_target : Error<
  "function using ZA state requires 'sme'">;
 def err_sme_definition_using_zt0_in_non_sme2_target : Error<
  "function using ZT0 state requires 'sme2'">;
+def warn_sme_streaming_pass_return_vl_to_non_streaming : Warning<
+  "passing a VL-dependent argument to/from a function that has a different"
+  " streaming-mode, the streaming and non-streaming vector lengths may be"


Suggested change

" streaming-mode, the streaming and non-streaming vector lengths may be"

" streaming-mode. The streaming and non-streaming vector lengths may be"

sdesmalen-arm · 2024-02-29T14:19:05Z

clang/lib/Sema/SemaChecking.cpp

+      if (CallerFnType != ArmStreaming &&
+          CallerFnType != ArmStreamingCompatible) {
+        if (IsCalleeStreaming && AnyScalableArgsOrRet)
+          Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);
+      } else if (CallerFnType == ArmStreaming && !IsCalleeStreaming &&
+                 !IsCalleeStreamingCompatible) {
+        if (AnyScalableArgsOrRet)
+          Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);
+      } else if (CallerFnType == ArmStreamingCompatible) {
+        if ((IsCalleeStreaming || !IsCalleeStreamingCompatible) &&
+            AnyScalableArgsOrRet)
+          Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming);
+      }


Do you think you can simplify this code?

sdesmalen-arm · 2024-02-29T14:32:30Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+  "passing a VL-dependent argument to/from a function that has a different"
+  " streaming-mode, the streaming and non-streaming vector lengths may be"
+  " different">,
+  InGroup<AArch64SMEAttributes>;


These warnings should be disabled by default, as the user may intentionally pass/return scalable vectors, knowing something about the target they're compiling for.

sdesmalen-arm · 2024-02-29T14:33:56Z

clang/lib/Sema/SemaDecl.cpp

  if (DeclIsDefn) {
    const auto *Attr = NewFD->getAttr<ArmNewAttr>();
    bool UsesSM = NewFD->hasAttr<ArmLocallyStreamingAttr>();
    bool UsesZA = Attr && Attr->isNewZA();
    bool UsesZT0 = Attr && Attr->isNewZT0();
+
+    if (UsesSM) {


Please write this without using UsesSM, since that variable is updated further down the line. It seems better to explicitly for NewFD->hasAttr<ArmLocallyStreamingAttr>().

sdesmalen-arm · 2024-03-04T14:06:55Z

clang/lib/Sema/SemaChecking.cpp

+    // non-compitable streaming attribute. If it passed any VL-based arguments
+    // or return VL-based value, then warn that the streaming and non-streaming
+    // vector lengths may be different.
+    if (CallerFD && Context.getTargetInfo().hasFeature("sme") && !IsBuiltin) {


Done.

It is not.

sdesmalen-arm · 2024-03-04T14:08:04Z

clang/lib/Sema/SemaChecking.cpp

-      }
+      if ((CallerFnType != ArmStreaming &&
+           CallerFnType != ArmStreamingCompatible && IsCalleeStreaming &&
+           AnyScalableArgsOrRet) ||


Can you factor out AnyScalableArgsOrRet?

sdesmalen-arm

Thanks for the update. I just left a few more suggestions to clean up the code a bit.

sdesmalen-arm · 2024-03-18T16:47:27Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+  "passing/returning a VL-dependent argument from a function"
+  " arm_locally_streaming attribute. The streaming and non-streaming vector"


Suggested change

"passing/returning a VL-dependent argument from a function"

" arm_locally_streaming attribute. The streaming and non-streaming vector"

"passing/returning a VL-dependent argument from a __arm_locally_streaming"

" function. The streaming and non-streaming vector"

sdesmalen-arm · 2024-03-18T16:48:59Z

clang/lib/Sema/SemaChecking.cpp

@@ -7513,6 +7516,30 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
      }
    }

+    auto *CallerFD = dyn_cast<FunctionDecl>(CurContext);
+    bool IsCalleeStreaming =
+        (ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask);


nit: parentheses are unnecessary.

Suggested change

(ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask);

ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask;

Also, please move these closer to their use (inside the if blocks)

sdesmalen-arm · 2024-03-18T16:49:41Z

clang/lib/Sema/SemaChecking.cpp

+    bool IsCalleeStreamingCompatible =
+        (ExtInfo.AArch64SMEAttributes &
+         FunctionType::SME_PStateSMCompatibleMask);


nit: parentheses are unnecessary.

Suggested change

bool IsCalleeStreamingCompatible =

(ExtInfo.AArch64SMEAttributes &

FunctionType::SME_PStateSMCompatibleMask);

bool IsCalleeStreamingCompatible =

ExtInfo.AArch64SMEAttributes &

FunctionType::SME_PStateSMCompatibleMask;

sdesmalen-arm · 2024-03-18T16:49:59Z

clang/lib/Sema/SemaChecking.cpp

+    bool IsCalleeStreamingCompatible =
+        (ExtInfo.AArch64SMEAttributes &
+         FunctionType::SME_PStateSMCompatibleMask);
+    bool IsBuiltin = (FD && FD->getBuiltinID());


nit: parentheses are unnecessary.

Suggested change

bool IsBuiltin = (FD && FD->getBuiltinID());

bool IsBuiltin = FD && FD->getBuiltinID();

sdesmalen-arm · 2024-03-18T16:51:25Z

clang/lib/Sema/SemaChecking.cpp

+        (ExtInfo.AArch64SMEAttributes &
+         FunctionType::SME_PStateSMCompatibleMask);
+    bool IsBuiltin = (FD && FD->getBuiltinID());
+    AnyScalableArgsOrRet |= Proto->getReturnType()->isSizelessVectorType();


Why not initialise the variable with Proto->getReturnType()->isSizelessVectorType() and then remove this separate |= statement?

sdesmalen-arm · 2024-03-18T16:53:58Z

clang/lib/Sema/SemaChecking.cpp

@@ -7513,6 +7516,30 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
      }
    }

+    auto *CallerFD = dyn_cast<FunctionDecl>(CurContext);


nit: move this closer to its use, preferably wrap the condition, e.g.

if (auto *CallerFD = dyn_cast<..>(..)) { ... }

sdesmalen-arm · 2024-03-19T08:43:47Z

clang/lib/Sema/SemaChecking.cpp

+      if ((CallerFnType != ArmStreaming &&
+           CallerFnType != ArmStreamingCompatible && IsCalleeStreaming) ||
+          (CallerFnType == ArmStreaming && !IsCalleeStreaming &&
+           !IsCalleeStreamingCompatible) ||
+          (CallerFnType == ArmStreamingCompatible &&
+           (IsCalleeStreaming || !IsCalleeStreamingCompatible)))


This can be simplified to:

if (!IsCalleeStreamingCompatible && (CallerFnType == ArmStreamingCompatible || ((CallerFnType == ArmStreaming) ^ IsCalleeStreaming)))

sdesmalen-arm · 2024-03-19T08:47:58Z

clang/lib/Sema/SemaChecking.cpp

+    // If the caller is a function and the callee has a different
+    // non-compitable streaming attribute. If it passed any VL-based arguments
+    // or return VL-based value, then warn that the streaming and non-streaming
+    // vector lengths may be different.


This comment reads a bit strange. What about:

// If the call requires a streaming-mode change and has scalable vector // arguments or return values, then warn the user that the streaming and // non-streaming vector lengths may be different.

sdesmalen-arm · 2024-03-19T16:15:46Z

clang/lib/Sema/SemaChecking.cpp

+    // If the call requires a streaming-mode change and has scalable vector
+    // arguments or return values, then warn the user that the streaming and
+    // non-streaming vector lengths may be different.
+    bool IsBuiltin = FD && FD->getBuiltinID();


minor nit: this definition can be moved to inside the if (auto *CallerFD = ..) block.

Done.

It's not though.

sdesmalen-arm · 2024-03-19T16:18:52Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3723,8 +3723,8 @@ def warn_sme_streaming_pass_return_vl_to_non_streaming : Warning<
  " different">,
  InGroup<AArch64SMEAttributes>, DefaultIgnore;
 def warn_sme_locally_streaming_has_vl_args_returns : Warning<
-  "passing/returning a VL-dependent argument from a function"
-  " arm_locally_streaming attribute. The streaming and non-streaming vector"
+  "passing/returning a VL-dependent argument from a arm_locally_streaming"


Suggested change

"passing/returning a VL-dependent argument from a arm_locally_streaming"

"passing/returning a VL-dependent argument from a __arm_locally_streaming"

sdesmalen-arm · 2024-03-25T15:31:33Z

clang/include/clang/Basic/DiagnosticGroups.td

@@ -1390,6 +1390,9 @@ def MultiGPU: DiagGroup<"multi-gpu">;
 // libc and the CRT to be skipped.
 def AVRRtlibLinkingQuirks : DiagGroup<"avr-rtlib-linking-quirks">;

+// A warning group AArch64 related to SME function attribues.


nit: Sorry, just spotted this.

Suggested change

// A warning group AArch64 related to SME function attribues.

// A warning group related to AArch64 SME function attribues.

sdesmalen-arm · 2024-03-25T15:32:44Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+  " different">,
+  InGroup<AArch64SMEAttributes>, DefaultIgnore;
+def warn_sme_locally_streaming_has_vl_args_returns : Warning<
+  "passing/returning a VL-dependent argument from a __arm_locally_streaming"


Suggested change

"passing/returning a VL-dependent argument from a __arm_locally_streaming"

"passing/returning a VL-dependent argument to/from a __arm_locally_streaming"

sdesmalen-arm · 2024-03-25T15:34:49Z

clang/lib/Sema/SemaChecking.cpp

+    // If the call requires a streaming-mode change and has scalable vector
+    // arguments or return values, then warn the user that the streaming and
+    // non-streaming vector lengths may be different.
+    bool IsBuiltin = FD && FD->getBuiltinID();


Done.

It's not though.

sdesmalen-arm · 2024-04-04T09:52:04Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+def warn_sme_streaming_pass_return_vl_to_non_streaming : Warning<
+  "passing a VL-dependent argument to/from a function that has a different"
+  " streaming-mode. The streaming and non-streaming vector lengths may be"
+  " different">,
+  InGroup<AArch64SMEAttributes>, DefaultIgnore;
+def warn_sme_locally_streaming_has_vl_args_returns : Warning<
+  "passing/returning a VL-dependent argument to/from a __arm_locally_streaming"
+  " function. The streaming and non-streaming vector"
+  " lengths may be different">,
+  InGroup<AArch64SMEAttributes>, DefaultIgnore;


Personally I would prefer it if you could give these warnings a 'bool' operand, such that it would give a message about either the return value or the operand, for example:

def warn_sme_streaming_pass_return_vl_to_non_streaming : Warning< "%select{passing|returning}0 a VL-dependent argument %select{to|from}0 a function with a different" " streaming-mode is undefined behaviour if the streaming and non-streaming vector lengths are different at runtime">, InGroup<AArch64SMEAttributes>, DefaultIgnore;

And then emit the diagnostic like this:

if (ScalableArgs) Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming) << /*IsArg=*/true; if (ScalableRet) Diag(Loc, diag::warn_sme_streaming_pass_return_vl_to_non_streaming) << /*IsArg=*/false;

This results in:

warning: returning a VL-dependent argument from a function with a different streaming-mode is undefined behaviour if the streaming and non-streaming vector lengths are different at runtime

and

warning: passing a VL-dependent argument to a function with a different streaming-mode is undefined behaviour if the streaming and non-streaming vector lengths are different at runtime

I won't hold up the patch for it, but I would appreciate it if you could do this either as part of this PR or as a follow-up patch.

…or size might be different. The compiler doesn't know in advance if the streaming and non-streaming vector-lengths are different, so it should be safe to give a warning diagnostic to warn the user about possible undefined behaviour. If the user knows the vector lengths are equal, they can disable the warning separately.

streaming local function from streaming function.

bgra8 · 2024-04-17T11:56:40Z

@dtemirbulatov this patch causes clang to crash. Please revert.

repro.cc:

typedef __SVFloat32_t a;
#pragma clang attribute push(__attribute__((target("+sve"))),                  \
                             apply_to = function)
template <int, int> struct b {
  using c = int;
};
template <class q> using e = q::c;
template <int f, int l> a g(b<f, l>, float);
template <class q> using h = decltype(g(q(), e<q>()));
h<b<0, 0>> i(b<0, 0>);
template <class q> using j = decltype(i(q()));
template <class k> struct m {
  static void n() {
    b<0, 0> d;
    k()(float(), d);
  }
};
template <class k> struct p {
  template <typename c, class q> void operator()(c, q p2) {
    int o;
    int misalignments[]{};
    for (int ma : misalignments)
      for (int mb : misalignments)
        k()(p2, 0, ma, mb, o);
  }
};
struct D {
  template <class q> void operator()(q, int, int, int, int) {
    [](j<q>) {};
  }
  void r() { m<p<D>>::n; }
};
#pragma clang attribute pop

Compilation command:

clang -cc1 -triple aarch64-unknown-linux-gnu  \
  -fsyntax-only \
  -std=gnu++20 \
  -x c++ \
  repro.cc

dtemirbulatov · 2024-04-17T13:40:18Z

Reproduced the issue.

bgra8 · 2024-04-17T14:05:15Z

@dtemirbulatov could you please revert to green to unblock us and continue the investigation asynchronously?

dtemirbulatov · 2024-04-17T15:21:43Z

ok, Reverting now.

… size difference (#79842)" This reverts commit 4e85e1f

dtemirbulatov · 2024-04-17T15:33:48Z

Reverted with 950bb09

…r size difference (#79842)" This reverts commit 950bb09

…r size difference (llvm#79842)" This reverts commit 950bb09

dtemirbulatov requested review from SamTebbs33, sdesmalen-arm, kmclaughlin-arm and MDevereau January 29, 2024 14:50

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 29, 2024

sdesmalen-arm reviewed Feb 2, 2024

View reviewed changes

sdesmalen-arm reviewed Feb 9, 2024

View reviewed changes

sdesmalen-arm reviewed Feb 14, 2024

View reviewed changes

dtemirbulatov requested a review from huntergr-arm February 22, 2024 12:54

sdesmalen-arm reviewed Feb 29, 2024

View reviewed changes

sdesmalen-arm reviewed Mar 4, 2024

View reviewed changes

sdesmalen-arm reviewed Mar 19, 2024

View reviewed changes

sdesmalen-arm reviewed Mar 25, 2024

View reviewed changes

sdesmalen-arm approved these changes Apr 4, 2024

View reviewed changes

Dinar Temirbulatov added 13 commits April 9, 2024 13:46

Corrected diagnostics, allowed to emit warning even when we call

e068d2f

streaming local function from streaming function.

Resolved comments.

8b6352d

Add new waring group AArch64SMEAttributes.

6f6ff8a

Resolved comments.

4b09435

Resolve comments.

00e3132

Updated comment in clang/lib/Sema/SemaDecl.cpp

d6367d0

Resolve comments.

c4c9155

Resolve remaining comments.

8b43957

Change all AArch64SMEAttributes warnings off by default.

b3aa561

Resolve comments.

d88118c

Resolve comment.

ef679b5

Resolved missed comment at llvm#7519

2cb1c3d

Resolve comments.

e3c81c9

dtemirbulatov force-pushed the sme-warn-call branch from b255c62 to e3c81c9 Compare April 9, 2024 14:31

dtemirbulatov merged commit 4e85e1f into llvm:main Apr 10, 2024

dtemirbulatov deleted the sme-warn-call branch April 10, 2024 07:49

dtemirbulatov pushed a commit that referenced this pull request Apr 17, 2024

Revert "[Clang][AArch64] Warn when calling non/streaming about vector…

950bb09

… size difference (#79842)" This reverts commit 4e85e1f

dtemirbulatov pushed a commit that referenced this pull request Apr 19, 2024

Reapply "[Clang][AArch64] Warn when calling non/streaming about vecto…

3a4bc11

…r size difference (#79842)" This reverts commit 950bb09

aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 21, 2024

Reapply "[Clang][AArch64] Warn when calling non/streaming about vecto…

769b499

…r size difference (llvm#79842)" This reverts commit 950bb09

		@@ -12239,7 +12239,8 @@ bool Sema::CheckFunctionDeclaration(Scope S, FunctionDecl NewFD,

		if (UsesSM) {

		(ExtInfo.AArch64SMEAttributes &
		FunctionType::SME_PStateSMCompatibleMask));

	if (CallerFD && Context.getTargetInfo().hasFeature("sme") && !IsBuiltin) {
	if (CallerFD && !IsBuiltin) {

	" streaming-mode, the streaming and non-streaming vector lengths may be"
	" streaming-mode. The streaming and non-streaming vector lengths may be"

		"passing/returning a VL-dependent argument from a function"
		" arm_locally_streaming attribute. The streaming and non-streaming vector"

	(ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask);
	ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask;

	bool IsBuiltin = (FD && FD->getBuiltinID());
	bool IsBuiltin = FD && FD->getBuiltinID();

	// A warning group AArch64 related to SME function attribues.
	// A warning group related to AArch64 SME function attribues.

[Clang][AArch64] Warn when calling streaming/non-streaming about vect… #79842

[Clang][AArch64] Warn when calling streaming/non-streaming about vect… #79842

Uh oh!

Conversation

dtemirbulatov commented Jan 29, 2024

Uh oh!

llvmbot commented Jan 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtemirbulatov Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtemirbulatov Feb 6, 2024 •

edited

Loading