[PATCH] D51650: Implement target_clones multiversioning

[Aaron Ballman via Phabricator via cfe-commits]

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