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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:3082
+static bool ArmMveAliasValid(unsigned BuiltinID, StringRef AliasName) {
+  // This will be filled in by Tablegen which isn't written yet
+  return false;
----------------
simon_tatham wrote:
> aaron.ballman wrote:
> > Comment should have a `FIXME` prefix, but tbh, I'm a little uncomfortable adopting the attribute without this being implemented. I assume the plan is to tablegen a header file that gets included here to provide the lookup?
> Yes: D67161, which I intend to commit as part of the same patch series once everything in it is approved, fills in this function with tablegen output.
> 
> I could roll both into the same monolithic patch, but I thought it would be better to break it up into manageable pieces as far as possible, especially when some of them (like this one) would need to be reviewed by people with significantly different expertise from the rest.
Ah, I didn't realize that was so involved; keeping them split makes sense.


================
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:
> > 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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7442
+  case ParsedAttr::AT_ClangBuiltinOverride:
+    handleClangBuiltinOverrideAttribute(S, D, AL);
+    break;
----------------
simon_tatham wrote:
> aaron.ballman wrote:
> > You should be able to call `handleSimpleAttribute<ClangBuiltinOverrideAttr>()` instead.
> Oh, nearly forgot: apparently I can't, because that template expects the attribute to have a constructor that takes only an `ASTContext` and an `AttributeCommonInfo`. But the constructor for my attribute also takes an `IdentifierInfo` giving the builtin name.
Ah, good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67159