[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Some drive-by nits.
General observation: this is kinda large.
I would //personally// split split off the codegen part into a second patch.



================
Comment at: include/clang/AST/Decl.h:2212-2213
 
+  bool isCPUDispatchMultiVersion() const;
+  bool isCPUSpecificMultiVersion() const;
+
----------------
Everything else has comments, and i'm not sure it is obvious what those mean without knowing the context.


================
Comment at: include/clang/Basic/AttrDocs.td:223
+A dispatching (or resolving) function can be declared anywhere in your
+compilation with ``cpu_dispatch``. This attribute takes one or more CPU names
+as a parameter (like ``cpu_specific``). Functions marked with ``cpu_dispatch``
----------------
s/compilation/source code/?


================
Comment at: include/clang/Basic/X86Target.def:288
+
+// FIXME: When commented out features are supported in LLVM, enable them here.
+CPU_SPECIFIC("generic", 'A', "")
----------------
This is going to go stale.
It already does not have znver1, btver2.
Surely this info already exists elsewhere in llvm?
Can it be deduplicated somehow?


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2421
+  // Main function's basic block.
+  llvm::BasicBlock *CurBlock = createBasicBlock("entry", Resolver);
+  Builder.SetInsertPoint(CurBlock);
----------------
Those are arbitrary names, right?
Maybe somehow convey that this is CPUDispatchMultiVersionResolver logic in the names?


================
Comment at: lib/CodeGen/CodeGenModule.cpp:868
+  const auto &Target = CGM.getTarget();
+  return std::string(".") + Target.CPUSpecificManglingCharacter(Name);
+}
----------------
I think
```
return Twine(".", Target.CPUSpecificManglingCharacter(Name)).str();
```


https://reviews.llvm.org/D47474