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