[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning

[Aaron Ballman via Phabricator via cfe-commits]

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