Page MenuHomePhabricator

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

Authored by sanjoy on Feb 10 2016, 12:40 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 47506.Feb 10 2016, 12:40 PM
sanjoy retitled this revision from to Identify cpu family 6 and model 94 as "skl" in getHostCPUName.
sanjoy updated this object.
sanjoy added reviewers: craig.topper, delena.
sanjoy added a subscriber: llvm-commits.
DavidKreitzer added inline comments.Feb 11 2016, 11:47 AM
lib/Support/Host.cpp
378 ↗(On Diff #47506)

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 added inline comments.Feb 11 2016, 3:31 PM
lib/Support/Host.cpp
378 ↗(On Diff #47506)

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 added inline comments.Feb 12 2016, 3:51 PM
lib/Support/Host.cpp
378 ↗(On Diff #47506)

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 updated this revision to Diff 47880.Feb 12 2016, 5:21 PM
  • address review
sanjoy retitled this revision from Identify cpu family 6 and model 94 as "skl" in getHostCPUName to Fix LLVM's handling and detection of skylake CPUs.Feb 12 2016, 5:24 PM
sanjoy updated this object.

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?

spatel added a subscriber: spatel.Feb 12 2016, 5:40 PM

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?

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.

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

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

  • Elena

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 updated this revision to Diff 48083.Feb 16 2016, 10:27 AM
  • address review
sanjoy retitled this revision from Fix LLVM's handling and detection of skylake CPUs to Fix LLVM's handling and detection of skylake and cannonlake CPUs.Feb 16 2016, 10:28 AM
sanjoy updated this object.
delena accepted this revision.Feb 20 2016, 11:48 PM
delena edited edge metadata.

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

This revision is now accepted and ready to land.Feb 20 2016, 11:48 PM
This revision was automatically updated to reflect the committed changes.