aaron.ballman added inline comments.
================ Comment at: include/clang/AST/Decl.h:2212-2213 + bool isCpuDispatchMultiVersion() const; + bool isCpuSpecificMultiVersion() const; + ---------------- aaron.ballman wrote: > Pedantic nit: CPU instead of Cpu? Thoughts on `isCPUDispatchMultiVersion()` instead of `isCpuDispatchMultiVersion()`? ================ Comment at: include/clang/Basic/Attr.td:851 + let Spellings = [Clang<"cpu_specific">]; + let Args = [VariadicIdentifierArgument<"Cpus">]; + let Subjects = SubjectList<[Function]>; ---------------- `Cpus` -> `CPUs` ? ================ Comment at: include/clang/Basic/Attr.td:865 + let Spellings = [Clang<"cpu_dispatch">]; + let Args = [VariadicIdentifierArgument<"Cpus">]; + let Subjects = SubjectList<[Function]>; ---------------- `Cpus` -> `CPUs` ? ================ Comment at: include/clang/Basic/Attr.td:851 + let Spellings = [GCC<"cpu_specific">]; + let Args = [VariadicIdentifierArgument<"Cpus">]; + let Subjects = SubjectList<[Function]>; ---------------- erichkeane wrote: > aaron.ballman wrote: > > erichkeane wrote: > > > aaron.ballman wrote: > > > > 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? > > > I'll add it, presumably as 'clang::cpu_specific'. The decision for > > > string-literal vs identifier was made quite a few years before I was here > > > sadly. I believe the EDG FE doesn't make identifiers any more difficult > > > so the implementer here chose to make it that way. > > > > > > In this case, I'd very much like to keep it with the same implementation > > > as ICC, simply because users of this are already familiar with it in this > > > form. > > > In this case, I'd very much like to keep it with the same implementation > > > as ICC, simply because users of this are already familiar with it in this > > > form. > > > > The compatibility with ICC is important for the GNU-style attribute, but > > for the C++ spelling this is novel territory where there is no > > compatibility story. Another approach to consider is whether to accept > > identifiers or string literals depending on the spelling, but that might > > not be worth it. > I'd like to think about that... I could forsee accepting BOTH forms, simply > because it would slightly simplify the conversion from an attribute-target-mv > situation, though I'm not sure it is important enough to do. > > I'm okay with the current approach that uses identifiers only. We can relax the rule to allow string literals if it turns out there is user demand for such a thing. ================ Comment at: include/clang/Basic/AttrDocs.td:247 +It is also possible to specify a CPU name of ``generic``, which is the +condition-less implementation, which will be resolved if the executing processor +doesn't satisfy the features required in the CPU name. The behavior of a program ---------------- I'd drop the bit about "which is the condition-less implementation". It reads a bit oddly to begin with and doesn't really add much to the explanation. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:866 + StringRef Name) { + const auto &Target = CGM.getTarget(); + return (Twine('.') + Twine(Target.CPUSpecificManglingCharacter(Name))).str(); ---------------- Don't use `auto` here. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:2446 + const auto *FD = cast<FunctionDecl>(GD.getDecl()); + assert(FD && "Not a FunctionDecl?"); + const auto *DD = FD->getAttr<CPUDispatchAttr>(); ---------------- `cast<>` already asserts this. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:2457 + false); + llvm::Function *ResolverFunc = cast<llvm::Function>( + GetOrCreateLLVMFunction(ResolverName, ResolverType, GlobalDecl{}, ---------------- Can use `auto *` here. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:2464 + const TargetInfo &Target = getTarget(); + for (IdentifierInfo *II : DD->cpus()) { + // Get the name of the target function so we can look it up/create it. ---------------- `const IdentifierInfo *` ================ Comment at: lib/Sema/Sema.cpp:1733 +static bool IsCPUDispatchCPUSpecificMultiVersion(Expr *E) { + if (const auto *UO = dyn_cast<UnaryOperator>(E)) ---------------- `const Expr *`? ================ Comment at: lib/Sema/SemaDecl.cpp:9587 + } else if (NewMVType == MultiVersioning::CPUSpecific && CurCPUSpec) { + + if (CurCPUSpec->cpus_size() == NewCPUSpec->cpus_size() && ---------------- Spurious newline? Also, `else` after a return. ================ Comment at: lib/Sema/SemaDecl.cpp:9614 + } + // if The two decls aren't the same MVType, there is no possible error + // condition. ---------------- s/if The/If the ================ Comment at: lib/Sema/SemaDecl.cpp:9639 return false; + +} ---------------- Spurious newline. ================ Comment at: lib/Sema/SemaDecl.cpp:9709 + // Previous declarations lack CPUDispatch/CPUSpecific. + else if (!OldFD->isMultiVersion()) { + S.Diag(OldFD->getLocation(), diag::err_multiversion_required_in_redecl) ---------------- `else` after `return`. ================ Comment at: lib/Sema/SemaOverload.cpp:9017 + + return (*FirstDiff.first)->getName() < (*FirstDiff.second)->getName(); + } ---------------- If there's no mismatch, doesn't this wind up dereferencing the end iterator of the range? https://reviews.llvm.org/D47474 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits