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

[Aaron Ballman via Phabricator via cfe-commits]

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