Page MenuHomePhabricator

Add check for AVX512 support before assuming skylake processor is SKX.
ClosedPublic

Authored by vchuravy on Jan 2 2017, 6:32 PM.

Details

Summary

Intel's i5-6300U CPU is reporting to have a model id of 78 (4e).
The Host detection assumes that to be Skylake Xeon (with AVX512 support),
instead of a normal Skylake machine.

This PR adds a check to verify that AVX512 support is indeed present.
The cpuinfo of an affected machine [1].

[1]: https://gist.github.com/nalimilan/8f74972a9f3a6f296dede7b56536f3ff

Patch by: Valentin Churavy

Event Timeline

vchuravy updated this revision to Diff 82827.Jan 2 2017, 6:32 PM
vchuravy retitled this revision from to Add check for AVX512 support before assuming skylake processor is SKX..
vchuravy updated this object.
nalimilan accepted this revision.Jan 3 2017, 12:53 AM
nalimilan added a reviewer: nalimilan.
nalimilan added a subscriber: nalimilan.

Fixes the incorrect detection of my i5-6300U CPU.

This revision is now accepted and ready to land.Jan 3 2017, 12:53 AM

(Though for clarity you could remove the // "skylake-avx512" comment and replace it with one comment for each branch.)

vchuravy updated this revision to Diff 82848.Jan 3 2017, 1:17 AM
vchuravy edited edge metadata.

groups the model id for skylake and checks for AVX512 to differentiate between SKL and SKX

Related issue for clang https://llvm.org/bugs/show_bug.cgi?id=27003 shows that the model id for i5-6200U is also 4e. I also grouped the two Skylake ids together.

I think 0x4e and 0x5e should both be "skylake" and 0x55 should be added for "skylake-avx512". At least with some google searching that seems to be what the linux kernel thinks the model numbers are.

vchuravy updated this revision to Diff 83022.Jan 4 2017, 2:08 AM

Add model id 0x55 as SKX and consolidate 0x4e & 0x5e into SKL

This follows the Linux kernel [1].

[1] https://github.com/torvalds/linux/blob/08328814256d888634ff15ba8fb67e2ae4340b64/arch/x86/include/asm/intel-family.h#L43-L45

@craig.topper should we still have a feature gate for 0x55? SKX is not on the market yet so we can't verify this yet.

tkelman added a subscriber: tkelman.Jan 4 2017, 5:01 AM

Go ahead and add the feature check for 0x55. That's probably safest. And it's the only difference between "skylake" and "skylake-avx512".

I recommend anyone using getHostCPUName to also use getHostCPUFeatures to truly be safe. The are low end 0x4e and 0x5e processors in the world that don't even support AVX1. We used to have code in getHostCPUName that downgraded any such processors all the way down to "nehalem" which removed a bunch of other features and changed the scheduling model. Thus why getHostCPUFeatures is recommended so you can get the CPU name for the scheduling model, but the features come from proper detection.

Is the skylake-avx512 family even worth having? Wouldn't it be better in all cases to check specifically for AVX-512 support?

vchuravy updated this revision to Diff 83135.Jan 4 2017, 2:12 PM

SKX: Defensively check for AVX512

hfinkel added a subscriber: hfinkel.Jan 4 2017, 2:15 PM
hfinkel added inline comments.
lib/Support/Host.cpp
485

Should the:

// "skylake-avx512"

comment be moved down to where the Subtype is set?

vchuravy updated this revision to Diff 83137.Jan 4 2017, 2:22 PM

address review comments

craig.topper accepted this revision.Jan 4 2017, 8:19 PM
craig.topper added a reviewer: craig.topper.

LGTM

This revision was automatically updated to reflect the committed changes.