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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
