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