[Clang][AArch64] Add diagnostic for calls from non-ZA to shared-ZA functions.

[sdesmalen]

Summary

The caller is required to have ZA state if it wants to share it with a callee.

[rsandifo-arm]

clang/lib/Sema/SemaChecking.cpp

6823

Very minor, but it looked odd to me to have |= in this statement and = above.

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

181

Would be good to have some tests for indirect function calls too (via function pointers), to make sure that the diagnostic still works when no decl is available.

I suppose this applies to D157269 too.

[sdesmalen]

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

181

I'm not sure that's necessary because D127762 already added tests to ensure the attributes propagate on pointer types, which then sets the ExtProtoInfo for those values. This patch merely checks the SME attribute fields from ExtProtoInfo. i.e. there is already nothing depending on a declaration being available.

[rsandifo-arm]

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

181

But Sema::checkCall does have some tests that depend on the decl rather than the type. So the purpose of the test wouldn't be “does the attribute stick when applied to indirect function types?” (which I agree is already covered), but “does the new code correctly process the attribute on the target of a function pointer type?”

[sdesmalen]

• Replaced |= into normal assignment =
• Added test for global initializer.

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

181

The declaration is only relevant for the call-site, not the callee.

if (ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateZASharedMask) {

The above line checks __arm_shared_za attribute of the callee (could be a decl, or a function pointer, but in either case is a prototyped function with the propagated attributes)

if (auto *CallerFD = dyn_cast<FunctionDecl>(CurContext)) {

The above line checks if the call-site context is a FunctionDecl (or definition for that matter). If the call is not part of a declaration (e.g. it's part of some global initialiser), we know it cannot have any live ZA state (which I now realise is missing a test-case).

So I think that a test like this:

__arm_new_za void foo(void (*f)() __arm_shared_za) { f(); }

is not testing anything that isn't already tested. But perhaps I'm still misunderstanding your point. If so, could you give an example of a test you have in mind?

[rsandifo-arm]

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

181

That's the kind of test I had in mind.

checkCall does have some conditions that are based on the callee decl rather than the callee type. That is, it does distinguish between direct calls and indirect calls. It would have been easy for the code to be in a block that was guarded with FDecl. Or it could be accidentally rearranged like that in future, especially if the function grows to have several more tests.

And yeah, I agree that the code as-written works, and so it doesn't fall into the trap that is being tested for. But that's true of most tests that get written for new features. :)

So a test for both the direct and indirect cases seemed worthwhile. If we were just going to have one, then I think testing the indirect case is more valuable than testing the direct case, since it's the type rather than the decl that matters.

I won't press the point further though. Feel free to skip the comment if you'd rather keep the tests as they are.

[sdesmalen]

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

181

Thanks for elaborating, I was mostly trying to understand your reasoning for adding this test.

If we were just going to have one, then I think testing the indirect case is more valuable than testing the direct case, since it's the type rather than the decl that matters.

You're right that the tests should probably have used the indirect case from the start. I see your argument about the code possibly changing in the future. It seems unlikely that someone would add an extra FDecl guard above the block where I've added the code, but I see how that's not really the point. I guess there is little argument not to add another positive test-case to the patch, so I'll just do that.

[sdesmalen]

Added test-cases for indirect calls

[aaron.ballman]

LGTM aside from a nit.

clang/lib/Sema/SemaChecking.cpp

6819

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_vecreturn_only_vector_member : Error<
   "the vecreturn attribute can only be used on a class or structure with one member, which must be a vector">;
 def err_attribute_vecreturn_only_pod_record : Error<
   "the vecreturn attribute can only be used on a POD (plain old data) class or structure (i.e. no virtual functions)">;
 def err_sme_attr_mismatch : Error<
   "function declared %0 was previously declared %1, which has different SME function attributes">;
 def err_sme_call_in_non_sme_target : Error<
   "call to a streaming function requires 'sme'">;
+def err_sme_za_call_no_za_state : Error<
+  "call to a shared ZA function requires the caller to have ZA state">;
 def err_sme_definition_using_sm_in_non_sme_target : Error<
   "function executed in streaming-SVE mode requires 'sme'">;
 def err_sme_definition_using_za_in_non_sme_target : Error<
   "function using ZA state requires 'sme'">;
 def err_cconv_change : Error<
   "function declared '%0' here was previously declared "
   "%select{'%2'|without calling convention}1">;
 def warn_cconv_unsupported : Warning<

clang/lib/Sema/SemaChecking.cpp

if (ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateSMEnabledMask) {
         llvm::StringMap<bool> CallerFeatureMap;
         Context.getFunctionFeatureMap(CallerFeatureMap, CallerFD);
         if (!CallerFeatureMap.contains("sme"))
           Diag(Loc, diag::err_sme_call_in_non_sme_target);
       } else if (!Context.getTargetInfo().hasFeature("sme")) {
         Diag(Loc, diag::err_sme_call_in_non_sme_target);
       }
     }
+
+    // If the callee uses AArch64 SME ZA state but the caller doesn't define
+    // any, then this is an error.
+    if (ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateZASharedMask) {
+      bool CallerHasZAState = false;
+      if (const auto *CallerFD = dyn_cast<FunctionDecl>(CurContext)) {
+        if (CallerFD->hasAttr<ArmNewZAAttr>())
+          CallerHasZAState = true;
+        else if (const auto *FPT = CallerFD->getType()->getAs<FunctionProtoType>())
+          CallerHasZAState = FPT->getExtProtoInfo().AArch64SMEAttributes &
+                             FunctionType::SME_PStateZASharedMask;
+      }
+
+      if (!CallerHasZAState)
+        Diag(Loc, diag::err_sme_za_call_no_za_state);
+    }
   }
 
   if (FDecl && FDecl->hasAttr<AllocAlignAttr>()) {
     auto *AA = FDecl->getAttr<AllocAlignAttr>();
     const Expr *Arg = Args[AA->getParamIndex().getASTIndex()];
     if (!Arg->isValueDependent()) {
       Expr::EvalResult Align;
       if (Arg->EvaluateAsInt(Align, Context)) {

clang/test/CodeGen/aarch64-sme-intrinsics/aarch64-sme-attrs.cpp

 
 // Test that the attributes are propagated properly to calls
 // when using a variadic template as indirection.
 __attribute__((always_inline))
 int call() { return 0; }
 
 template <typename T, typename... Other>
 __attribute__((always_inline))
-int call(T f, Other... other) {
+int call(T f, Other... other) __arm_shared_za {
     return f() + call(other...);
 }
 
 // CHECK: {{.*}} @_Z22test_variadic_templatev(
 // CHECK:      call {{.*}} i32 @normal_callee() #[[NOUNWIND_CALL:[0-9]+]]
 // CHECK-NEXT: call {{.*}} i32 @streaming_decl() #[[NOUNWIND_SM_ENABLED_CALL:[0-9]+]]
 // CHECK-NEXT: call {{.*}} i32 @streaming_compatible_decl() #[[NOUNWIND_SM_COMPATIBLE_CALL:[0-9]+]]
 // CHECK-NEXT: call {{.*}} i32 @shared_za_decl() #[[NOUNWIND_ZA_SHARED_CALL:[0-9]+]]
 // CHECK-NEXT: call {{.*}} i32 @preserves_za_decl() #[[NOUNWIND_ZA_PRESERVED_CALL:[0-9]+]]
 // CHECK-NEXT: add nsw
 // CHECK-NEXT: add nsw
 // CHECK-NEXT: add nsw
 // CHECK-NEXT: add nsw
 // CHECK-NEXT: ret
-int test_variadic_template() {
+int test_variadic_template() __arm_shared_za {
   return call(normal_callee,
               streaming_decl,
               streaming_compatible_decl,
               shared_za_decl,
               preserves_za_decl);
 }
 
 // CHECK: attributes #[[SM_ENABLED]] = { mustprogress noinline nounwind "aarch64_pstate_sm_enabled" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+bf16,+sme" }

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

 
 // expected-error@+5 {{function declared 'void (void) __arm_shared_za __arm_preserves_za' was previously declared 'void (void) __arm_shared_za', which has different SME function attributes}}
 // expected-note@+3 {{previous declaration is here}}
 // expected-cpp-error@+3 {{function declared 'void () __arm_shared_za __arm_preserves_za' was previously declared 'void () __arm_shared_za', which has different SME function attributes}}
 // expected-cpp-note@+1 {{previous declaration is here}}
 void redecl_nopreserve_za(void) __arm_shared_za;
 void redecl_nopreserve_za(void) __arm_shared_za __arm_preserves_za {}
 
+void non_za_definition(void (*shared_za_fn_ptr)(void) __arm_shared_za) {
+  sme_arm_new_za(); // OK
+  // expected-error@+2 {{call to a shared ZA function requires the caller to have ZA state}}
+  // expected-cpp-error@+1 {{call to a shared ZA function requires the caller to have ZA state}}
+  sme_arm_shared_za();
+  // expected-error@+2 {{call to a shared ZA function requires the caller to have ZA state}}
+  // expected-cpp-error@+1 {{call to a shared ZA function requires the caller to have ZA state}}
+  shared_za_fn_ptr();
+}
+
+void shared_za_definition(void (*shared_za_fn_ptr)(void) __arm_shared_za) __arm_shared_za {
+  sme_arm_shared_za(); // OK
+  shared_za_fn_ptr(); // OK
+}
+
+__arm_new_za void new_za_definition(void (*shared_za_fn_ptr)(void) __arm_shared_za) {
+  sme_arm_shared_za(); // OK
+  shared_za_fn_ptr(); // OK
+}
+
 #ifdef __cplusplus
+int shared_za_initializer(void) __arm_shared_za;
+// expected-cpp-error@+1 {{call to a shared ZA function requires the caller to have ZA state}}
+int global = shared_za_initializer();
+
 struct S {
   virtual void shared_za_memberfn(void) __arm_shared_za;
 };
 
 struct S2 : public S {
 // expected-cpp-error@+2 {{virtual function 'shared_za_memberfn' has different attributes ('void ()') than the function it overrides (which has 'void () __arm_shared_za')}}
 // expected-cpp-note@-5 {{overridden virtual function is here}}
   __arm_new_za void shared_za_memberfn(void) override {}