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

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham marked 10 inline comments as done.
simon_tatham 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;
----------------
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.


================
Comment at: clang/lib/AST/Decl.cpp:3107
+    if (!ArmMveAliasValid(BuiltinID, getIdentifier()->getName())) {
+      getASTContext().getDiagnostics().Report(
+        getLocation(), diag::err_attribute_arm_mve_alias);
----------------
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.


================
Comment at: clang/test/CodeGen/arm-mve-intrinsics/alias-attr.c:1
+// RUN: %clang_cc1 -triple armv8.1m.main-arm-none-eabi -verify -fsyntax-only %s
+
----------------
aaron.ballman wrote:
> This seems like a Sema test, not a CodeGen test.
> 
> There are other Sema tests missing: the attribute only accepts one arg instead of zero or two+, only accepts identifier args (test with a string literal and an integer literal), only applies to functions, etc.
> 
> Should there be a diagnostic if the attribute is applied to a function with internal linkage (or are there static builtins)? What about member functions of a class?
> 
> Once the builtin hookup is complete, there should also be tests for the attribute being accepted properly, and codegen tests demonstrating the proper builtin is called.
Thanks for pointing out the rest of those missing test cases; I'll add them.

The //intended// use of this attribute is very similar to the example I show here (only with one of the upcoming MVE builtins in place of `__builtin_arm_nop`): declaring a function as `static __inline__` that corresponds to a builtin (either by name, or via this new attribute) has the effect of filling in the prototype of the function at compile time if it wasn't baked into `Builtins.def` up front. (Which I'll need to do for the MVE builtins, because some of them will have struct types as arguments that won't exist until the header file has defined them.)

I could add a diagnostic if the attribute is applied to a function with //external// linkage, and/or one for class member functions, if you think either one is important. (I don't expect to need either of those for my intended use case.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67159