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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
