[PATCH] D38824: [X86] Synchronize the CPU predefined macros with gcc

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.


================
Comment at: lib/Basic/Targets/X86.cpp:844-845
-    // FIXME: Historically, we defined this legacy name, it would be nice to
-    // remove it at some point. We've never exposed fine-grained names for
-    // recent primary x86 CPUs, and we should keep it that way.
-    defineCPUMacros(Builder, "corei7");
----------------
craig.topper wrote:
> chandlerc wrote:
> > This seems to undo the idea that we should keep avoiding exposing fine-grained CPU names? What's new that changes this?
> CPUs newer than the ones with that comment seem to have ignored said comment.
> 
> Probably be cause we don't have a definition for what to do for new CPUs if we aren't going to expose fine grained names. Do we just call everything corei7 forever?
My hope would have been that people use *ISA*-based macros rather than anything w/ a specific CPU. So I would have *removed* the corei7 for future CPUs and simply not defined anything for them at all.

I think exposing something beyond ISA featureset through preprocessor macros is questionable at best. I would at least want to see concrete use cases first.


================
Comment at: lib/Basic/Targets/X86.cpp:852
+    defineCPUMacros(Builder, "core_avx2");
+    defineCPUMacros(Builder, "haswell");
     break;
----------------
craig.topper wrote:
> chandlerc wrote:
> > I find calling a Westmere CPU `nehalem` a little odd. Calling IvyBridge a `sandybridge' CPU seems quite confusing. But calling Skylake (client) and Cannonlake (all? client?) `haswell` seems .... deeply weird.
> This implementation matches what gcc does. I agree its weird.
> 
> gcc doesn't implement cannonlake yet so i don't know what they'll do.
Ok, but maybe we should ask GCC to stop doing this. ;] We could talk to them and try to figure out the right strategy is. My proposed strategy (see other comment) is to restrict macros to ISA facilities.


https://reviews.llvm.org/D38824