aaron.ballman added inline comments.
================ Comment at: include/clang/AST/Decl.h:2212-2213 + bool isCpuDispatchMultiVersion() const; + bool isCpuSpecificMultiVersion() const; + ---------------- Pedantic nit: CPU instead of Cpu? ================ Comment at: include/clang/Basic/Attr.td:850 +def CpuSpecific : InheritableAttr { + let Spellings = [GCC<"cpu_specific">]; + let Args = [VariadicIdentifierArgument<"Cpus">]; ---------------- Does GCC really support this spelling? I couldn't find documentation to suggest it, and a quick test suggests it's not supported there. Perhaps this should use the Clang spelling instead? ================ Comment at: include/clang/Basic/Attr.td:851 + let Spellings = [GCC<"cpu_specific">]; + let Args = [VariadicIdentifierArgument<"Cpus">]; + let Subjects = SubjectList<[Function]>; ---------------- Be sure to add a test for using this attribute with the C++ spelling, as I'm not certain how well we would parse something like `[[gnu::cpu_specific(ivybridge)]]` currently (it may just work, however). Also, why an identifier instead of a string literal? ================ Comment at: include/clang/Basic/Attr.td:864 +def CpuDispatch : InheritableAttr { + let Spellings = [GCC<"cpu_dispatch">]; + let Args = [VariadicIdentifierArgument<"Cpus">]; ---------------- Same here. ================ Comment at: include/clang/Basic/AttrDocs.td:225-226 +as a parameter (like ``cpu_specific``). However, ``cpu_dispatch`` functions +may not have a body in its definition. An empty definition is permissible for +ICC compatibility, and all other definitions will have their body ignored. + ---------------- How empty is empty? e.g., is it lexically empty (no tokens whatsoever) or notionally empty (empty after preprocessing)? ``` __attribute__((cpu_dispatch(atom))) void multi_cpu_1(void) { /* Is this empty enough? */ } __attribute__((cpu_dispatch(atom))) void multi_cpu_2(void) { #if 0 #error How about this? #endif } ``` ================ Comment at: lib/Parse/ParseDecl.cpp:209 -/// Determine whether the given attribute has an identifier argument. +/// Determine whether the given attribute has a variadic identifier argument. static bool attributeHasIdentifierArg(const IdentifierInfo &II) { ---------------- This comment seems to be in the wrong place. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:1877-1878 +static void handleCpuSpecificAttr(Sema &S, Decl *D, const AttributeList &AL) { + FunctionDecl *FD = D->getAsFunction(); + assert(FD && "CPU Specific/Dispatch only valid on a Function Decl"); + ---------------- Better to just use `cast<FunctionDecl>(D)` here as it already asserts properly. ================ Comment at: utils/TableGen/ClangAttrEmitter.cpp:2130 + std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr"); + for (const auto *Attr : Attrs) { + // Determine whether the first argument is a variadic identifier. ---------------- Please don't use `Attr` as a local name; it's a type. https://reviews.llvm.org/D47474 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits