[PATCH] D16756: Added SKL and CNL processors and features to Clang

[David Kreitzer via llvm-commits]

DavidKreitzer added a subscriber: chandlerc.

================
Comment at: lib/Basic/Targets.cpp:2341
@@ -2322,3 +2340,3 @@
         .Case("broadwell", CK_Broadwell)
-        .Case("skylake", CK_Skylake)
-        .Case("skx", CK_Skylake) // Legacy name.
+        .Case("skl", CK_SkylakeClient)
+        .Case("skylake", CK_SkylakeServer)
----------------
Are the new names "skl" and "cnl" consistent with what gcc is planning/doing? I suspect that cannonlake might be spelled out, and I'm not at all sure about "skl".

I'd rather we not have to change this later, be burdened with more "Legacy name"s, and have to do additional cleanups like this one:

------------------------------------------------------------------------


> r223776 | chandlerc | 2014-12-09 06:50:25 -0800 (Tue, 09 Dec 2014) | 20 lines
> 
> Re-work the Clang system for classifying Intel x86 CPUs to use their
> basic microarchitecture names, and add support (with tests) for parsing
> all of the masic microarchitecture names for CPUs documented to be
> accepted by GCC with -march. I didn't go back through the 32-bit-only
> old microarchitectures, but this at least brings the recent architecture
> names up to speed. This is essentially the follow-up to the LLVM commit
> r223769 which did similar cleanups for the LLVM CPUs.
> 
> One particular benefit is that you can now use -march=westmere in Clang
> and get the LLVM westmere processor which is a different ISA variant (!)
> and so quite significant.
> 
> Much like with r223769, I would appreciate the Intel folks carefully
> thinking about the macros defined, names used, etc for the atom chips
> and newest primary x86 chips. The current patterns seem quite strange to
> me, especially here in Clang.
> 
> Note that I haven't replicated the per-microarchitecture macro defines
> provided by GCC. I'm really opposed to source code using these rather
> than using ISA feature macros.
> 
> 



================
Comment at: lib/Basic/Targets.cpp:2698
@@ -2661,3 +2697,3 @@
     setFeatureEnabledImpl(Features, "bmi2", true);
     setFeatureEnabledImpl(Features, "rtm", true);
     setFeatureEnabledImpl(Features, "fma", true);
----------------
Is this right? Does KNL support RTM?

================
Comment at: lib/Basic/Targets.cpp:3240
@@ -3182,2 +3239,3 @@
     break;
-  case CK_Skylake:
+  case CK_SkylakeClient:
+    defineCPUMacros(Builder, "skl");
----------------
These additions are in direct conflict with the FIXME and the spirit of r223776. At the very least, there needs to be some discussion before adding them. And like the -march settings, we should make sure we are consistent with what gcc is doing.

I agree with @chandlerc that programmers ought to be using the ISA macros, at least when the intent is to check for the existence of a feature. But I can see the utility of the tuning macros, e.g. _tune_slm_. Going forward, we might choose to add just those, i.e. _tune_cannonlake_ and not _cannonlake_.


================
Comment at: lib/Basic/Targets.cpp:3395
@@ -3331,1 +3394,3 @@
     Builder.defineMacro("__AVX512VL__");
+  if (HasAVX512VBMI)
+    Builder.defineMacro("__AVX512VBMI__");
----------------
Does gcc and/or icc define these same macros? Or at the very least, is there agreement on the the names so that we don't end up with inconsistencies? For example, I don't see any evidence that the MOVBE macro is in use even though the movbe feature has been around for a while.



Repository:
  rL LLVM

http://reviews.llvm.org/D16756