aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.
Thank you for this! I have some cursory review comments, and possibly more
later.
================
Comment at: include/clang/Basic/AttrDocs.td:1600
+ let Content = [{
+Clang supports the GNU style ``__attribute__((target_clones("OPTIONS")))``
+attribute. This attribute may be attached to a function definition and causes
----------------
It supports the C++ style as well, so perhaps "Clang supports the
``target_clones("OPTIONS")`` attribute." instead?
================
Comment at: include/clang/Basic/AttrDocs.td:1601
+Clang supports the GNU style ``__attribute__((target_clones("OPTIONS")))``
+attribute. This attribute may be attached to a function definition and causes
+function multiversioning, where multiple versions of the function will be
----------------
It can be on a declaration too, can't it?
================
Comment at: include/clang/Basic/AttrDocs.td:1602
+attribute. This attribute may be attached to a function definition and causes
+function multiversioning, where multiple versions of the function will be
+emitted with different code generation options. Additionally, these versions
----------------
Extraneous space between "versions of"
================
Comment at: include/clang/Basic/AttrDocs.td:1607
+
+All multiversioned functions must contain a ``default`` (fallback)
+implementation, otherwise usages of the function are considered invalid.
----------------
You should probably explain the options a bit more thoroughly around here;
nothing explains that the options are architectures, for instance.
================
Comment at: include/clang/Basic/AttrDocs.td:1612
+Note that unlike the ``target`` syntax, every option listed creates a new
+version, desregarding whether it is split on a comma inside or outside a
string.
+The following will emit 4 versions of the function.
----------------
typo: disregarding
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2394
const auto *F = cast<FunctionDecl>(GD.getDecl());
+
if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr<AlwaysInlineAttr>())
----------------
Spurious newline?
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2559
+ assert(FD && "Not a FunctionDecl?");
+ auto *ClonesAttr = FD->getAttr<TargetClonesAttr>();
+ assert(ClonesAttr && "Not a target_clones Function?");
----------------
`const auto *`
================
Comment at: lib/Sema/SemaDecl.cpp:9428
S.Diag(OldFD->getLocation(), diag::err_multiversion_no_other_attrs)
- << IsCPUSpecificCPUDispatchMVType;
+ << (MVType - 1);
S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
----------------
Rather than repeat this expression in multiple places, I prefer a named local
variable.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3014
+ while (Str.size() != 0) {
+ // remove the comma we found last time through.
+ if (Str[0] == ',')
----------------
remove -> Remove
https://reviews.llvm.org/D51650
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
