Fix LLVM's handling and detection of skylake and cannonlake CPUs

[sanjoy]

Summary

• Rename "skylake" == SkylakeServerProc to "skylake-avx512"
• Change "skylake" to denote SkylakeClientProc
• Fix the detection of cpu family 6 and model 94 to be SkylakeClientProc instead of SkylakeServerProc
• Remove the "cnl" for CannonLake

[DavidKreitzer]

lib/Support/Host.cpp

378

See http://reviews.llvm.org/D16756.

You probably want to replace "skylake" with "skylake-avx512" and "skl" with "skylake" to be consistent with the naming scheme gcc is using for the -march switch.

[sanjoy]

lib/Support/Host.cpp

378

You mean replace "skl" with "skylake" everywhere? Won't that break clang (as of today) since today by "skylake" it means Skylake Server? Or does clang not have to agree with LLVM on these names?

[DavidKreitzer]

lib/Support/Host.cpp

378

Right, I'm suggesting replacing "skl" with "skylake" everywhere and "skylake" with "skylake-avx512" everywhere (both in LLVM and in clang) to be consistent with gcc. I admit that might be beyond what you are trying to do in this change set, though.

I think you are correctly classifying the model 94 CPU as SkylakeClient, but without accompanying changes in clang to actually support a "skl" target (like Elena is doing in http://reviews.llvm.org/D16756.), won't this cause problems for clang? What happens if you run "clang -march=native" with this change on your SkylakeClient machine?

[sanjoy]

• address review

[sanjoy]

Hi David,

I've done the rename you suggested, and have added a dependency on Elena's D16756 change (indicating that these need to go in together). Does that sound right?

[DavidKreitzer]

Thanks for doing that, Sanjoy! Yes, that sounds like a good plan. And these changes all look good to me.

Elena, are there any other places we need to change to match?

[spatel]

Craig proposed a change in the post-commit thread for r258659 which may affect this patch. I agree with this:
"I don't like the idea of these wrapper processor feature sets. I don't like the SLM and Atom ones we already had. It always felt like the checks that were based on Atom/SLM should be based on -mcpu or -march and not a "feature". I don't think its a good idea to encourage more of this."

I would go even further: basing any codegen transform on a CPU model rather than some specific feature of that microarch is bad. Basing a code transform on a CPU's marketing name is double-bad. :)

Sooner or later, that kind of predicate will need fixing.

I think it's fine to have micro-arch and marketing name bundles up in clang, but I'd like to see X86ProcFamilyEnum disappear down here.

Related: we have a bunch of "FeatureSlow*" bits which aren't really features either. And although I've added to that collection myself, I think it would be better to sink some of those transforms to the machine instruction level. There was some discussion about that here:
https://llvm.org/bugs/show_bug.cgi?id=26183
Alternatively, we could do more DAG-level predication like this:
http://reviews.llvm.org/rL244601
or hoist things up to CodeGenPrepare and use TTI/TLI.

[llvm-commits]

We have
def : CannonlakeProc<"cannonlake">;
def : CannonlakeProc<"cnl">;

But "skl" is replaced with "skylake".
Should we remove "cnl" as well?

• Elena

[sanjoy]

In D17090#352306, @llvm-commits wrote:

We have
def : CannonlakeProc<"cannonlake">;
def : CannonlakeProc<"cnl">;

But "skl" is replaced with "skylake".
Should we remove "cnl" as well?

That makes sense (since in D16756 you use "cannonlake" as well). I'll fix this tomorrow.

• Elena

[sanjoy]

• address review

[delena]

LGTM.
I committed Clang changes: http://reviews.llvm.org/D16756. Now it should work together.

lib/Support/Host.cpp

case 6:
 
       // Broadwell:
       case 61:
       case 71:
         return "broadwell";
 
       // Skylake:
       case 78:
-      case 94:
         return "skylake";
+      case 94:
+        return "skl";
 
       case 28: // Most 45 nm Intel Atom processors
       case 38: // 45 nm Atom Lincroft
       case 39: // 32 nm Atom Medfield
       case 53: // 32 nm Atom Midview
       case 54: // 32 nm Atom Midview
         return "bonnell";