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

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added inline comments.


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:181
 
+void non_za_definition(void) {
+  sme_arm_new_za(); // OK
----------------
sdesmalen wrote:
> rsandifo-arm wrote:
> > sdesmalen wrote:
> > > rsandifo-arm wrote:
> > > > 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.
> > > 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.
> > 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?”
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157270/new/

https://reviews.llvm.org/D157270