[PATCH] D67159: [clang] New __attribute__((__clang_arm_mve_alias)).

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:3107
+    if (!ArmMveAliasValid(BuiltinID, getIdentifier()->getName())) {
+      getASTContext().getDiagnostics().Report(
+        getLocation(), diag::err_attribute_arm_mve_alias);
----------------
simon_tatham wrote:
> aaron.ballman wrote:
> > simon_tatham wrote:
> > > aaron.ballman wrote:
> > > > I'm not certain how comfortable I am with having this function produce a diagnostic. That seems like unexpected behavior for a function attempting to get a builtin ID. I think this should be the responsibility of the caller.
> > > The //caller//? But there are many possible callers of this function. You surely didn't mean to suggest duplicating the diagnostic at all those call sites.
> > > 
> > > Perhaps it would make more sense to have all the calculation in this `getBuiltinID` method move into a function called once, early in the `FunctionDecl`'s lifetime, which figures out the builtin ID (if any) and stashes it in a member variable? Then //that// would issue the diagnostic, if any (and it would be called from a context where that was a sensible thing to do), and `getBuiltinID` itself would become a mere accessor function.
> > > The caller? But there are many possible callers of this function. You surely didn't mean to suggest duplicating the diagnostic at all those call sites.
> > 
> > Yes, I did. :-) No caller is going to expect that calling a `const` function that gets a builtin ID is going to issue diagnostics and so this runs the risk of generating diagnostics in surprising situations, such as from AST matchers.
> > 
> > > Perhaps it would make more sense to have all the calculation in this getBuiltinID method move into a function called once, early in the FunctionDecl's lifetime, which figures out the builtin ID (if any) and stashes it in a member variable? Then that would issue the diagnostic, if any (and it would be called from a context where that was a sensible thing to do), and getBuiltinID itself would become a mere accessor function.
> > 
> > That might make sense, but I don't have a good idea of what performance concerns that might raise. If there are a lot of functions and we never need to check if they have a builtin ID, that could be expensive for little gain.
> OK – so actually what you meant to suggest was to put the diagnostic at just //some// of the call sites for `getBuiltinId`?
> 
> With the intended behavior being that the Sema test in this patch should still provoke all the expected diagnostics in an ordinary compilation context, but in other situations like AST matchers, it would be better for `getBuiltinId` to //silently// returns 0 if there's an illegal ArmMveAlias attribute?
> 
> (I'm just checking I've understood you correctly before I do the work...)
> OK – so actually what you meant to suggest was to put the diagnostic at just some of the call sites for getBuiltinId?

Yes! Sorry, I can see how I was unclear before. :-)

> With the intended behavior being that the Sema test in this patch should still provoke all the expected diagnostics in an ordinary compilation context, but in other situations like AST matchers, it would be better for getBuiltinId to silently returns 0 if there's an illegal ArmMveAlias attribute?

Yes. `getBuiltinId()` already returns `0` in error cases without diagnosing, such as the function being unnamed or not being a builtin. I want to retain that property -- this function returns zero if the function is not a builtin. It's up to the caller of the function to decide whether a zero return value should be diagnosed or not.

To be honest, this diagnostic feels like it belongs in SemaDeclAttr.cpp; it is placing a constraint on which declarations can have the attribute, so that should be checked *before* applying the attribute to the declaration. This also keeps the AST cleaner by not having an attribute on a function which should not be attributed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67159