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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits