[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 4 inline comments as done.
erichkeane added a comment.

In https://reviews.llvm.org/D47474#1154858, @lebedev.ri wrote:

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


Thanks for the review!  I definitely agree this is a sizable patch, I'm not sure the non-codegen part has any value without the codegen so I'm not sure that splitting it would provide value.  I can definitely do so if reviewers in general believe this is the right tack.



================
Comment at: include/clang/Basic/X86Target.def:288
+
+// FIXME: When commented out features are supported in LLVM, enable them here.
+CPU_SPECIFIC("generic", 'A', "")
----------------
lebedev.ri wrote:
> 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?
You'll note that this doesn't have any non-intel names :)   This is non-Intel processor friendly version of the ICC implementation of the same feature.  Because of this, I don't have a good feature list for those.

I'd searched in a few places for an alternate source for these names/feature lists. I was unable to find anywhere else that needs/has similar information, though any alternate sources are greatly appreciated.

I'll note that the name and mangling-name aren't going to be able to be removed, though perhaps there is a source of these features somewhere?


================
Comment at: lib/CodeGen/CodeGenModule.cpp:868
+  const auto &Target = CGM.getTarget();
+  return std::string(".") + Target.CPUSpecificManglingCharacter(Name);
+}
----------------
lebedev.ri wrote:
> I think
> ```
> return Twine(".", Target.CPUSpecificManglingCharacter(Name)).str();
> ```
A twine of char + a char is actually pretty challenging... I rewrote it without the intermediate string though.  I am generally unconcerned, since 2 chars is very much in the 'short string optimization' range of all implementations.


https://reviews.llvm.org/D47474