aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:596
}
+def ArmMveAlias : InheritableAttr, TargetSpecificAttr<TargetARM> {
+ let Spellings = [Clang<"__clang_arm_mve_alias">];
----------------
Add a newline before the attribute definition for some separation.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4350
+as clang builtins equivalent to the underlying name. For example,
+``arm_mve.h`` will declare the function ``vaddq_u32`` with
+``__attribute__((__clang_arm_mve_alias(__builtin_arm_mve_vaddq_u32)))``,
----------------
will declare -> declares
================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:453
+def err_attribute_arm_mve_alias : Error<
+ "__clang_arm_mve_alias attribute can only be used for Arm MVE builtins">;
+
----------------
How about: `"'__clang_arm_mve_alias' attribute can only be applied to an ARM
MVE builtin"`
================
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;
----------------
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?
================
Comment at: clang/lib/AST/Decl.cpp:3102-3103
+
+ if (hasAttr<ArmMveAliasAttr>()) {
+ IdentifierInfo *II = getAttr<ArmMveAliasAttr>()->getBuiltinName();
+ BuiltinID = II->getBuiltinID();
----------------
```
if (const auto *AMAA = getAttr<ArmMveAliasAttr>) {
IdentifierInfo *II = AMAA->getBuiltinName();
...
}
```
================
Comment at: clang/lib/AST/Decl.cpp:3107
+ if (!ArmMveAliasValid(BuiltinID, getIdentifier()->getName())) {
+ getASTContext().getDiagnostics().Report(
+ getLocation(), diag::err_attribute_arm_mve_alias);
----------------
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.
================
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
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67159/new/
https://reviews.llvm.org/D67159
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
