[X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them
AbandonedPublic

Authored by craig.topper on Oct 11 2017, 2:30 PM.

Details

Summary

We were using corei7 for a large swatch of Intel CPUs. Now we only define nehalem/westmere as corei7. CPUs newer than that don't get defined with anything.

Diff Detail

craig.topper created this revision.Oct 11 2017, 2:30 PM
chandlerc added inline comments.Oct 11 2017, 3:18 PM
lib/Basic/Targets/X86.cpp
848–849

This seems to undo the idea that we should keep avoiding exposing fine-grained CPU names? What's new that changes this?

853

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.

craig.topper added inline comments.Oct 11 2017, 3:46 PM
lib/Basic/Targets/X86.cpp
848–849

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?

853

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.

chandlerc added inline comments.Oct 11 2017, 4:12 PM
lib/Basic/Targets/X86.cpp
848–849

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.

853

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.

Only define "corei7" on nehalem/westmere to match gcc. Don't define anything for the CPUs newer than that. Add comments to the CPUs where gcc has two sets of defines and we have only one.

craig.topper retitled this revision from [X86] Synchronize the CPU predefined macros with gcc to [X86] Synchronize the existing CPU predefined macros with the cases that gcc defines them.Oct 13 2017, 1:57 PM
craig.topper edited the summary of this revision. (Show Details)
RKSimon added inline comments.Oct 26 2017, 11:49 AM
lib/Basic/Targets/X86.cpp
837

defines

Fix Simon's comment

So, doing research to understand the impact of this has convinced me we *really* need to stop doing this. Multiple libraries are actually trying to enumerate every CPU that has feature X for some feature X. =[[[ This, combined with the fundamental pattern of defining a precise macro for the CPU, leaves a time bomb where anyone that passes a new CPU to -march using some older headers will incorrectly believe features aren't available on *newer* CPUs. =[

Specific examples:
https://github.com/hwoarang/glibc/blob/master/sysdeps/x86/cpu-features.h#L263
https://github.com/boostorg/atomic/blob/boost-1.65.1/include/boost/atomic/detail/caps_gcc_x86.hpp#L30

I think my conclusion is that the best way forward is to officially stop defining CPU-specific macros, but to also continue defining corei7 macros on all CPUs newer than that for all time so that old headers using these macros for "feature detection" actually work.

Thoughts?

lib/Basic/Targets/X86.cpp
837

Nit picking detail: I would also capitalize GCC here and elsewhere.

So, doing research to understand the impact of this has convinced me we *really* need to stop doing this. Multiple libraries are actually trying to enumerate every CPU that has feature X for some feature X. =[[[ This, combined with the fundamental pattern of defining a precise macro for the CPU, leaves a time bomb where anyone that passes a new CPU to -march using some older headers will incorrectly believe features aren't available on *newer* CPUs. =[

Specific examples:
https://github.com/hwoarang/glibc/blob/master/sysdeps/x86/cpu-features.h#L263
https://github.com/boostorg/atomic/blob/boost-1.65.1/include/boost/atomic/detail/caps_gcc_x86.hpp#L30

That's, um, interesting.

OTOH, if there were good feature macros for these things (e.g. cpuid), I imagine that the library authors would have preferred to use them. I doubt the authors of that code really wanted to do it that way either.

I think my conclusion is that the best way forward is to officially stop defining CPU-specific macros, but to also continue defining corei7 macros on all CPUs newer than that for all time so that old headers using these macros for "feature detection" actually work.

Thoughts?

I think that we do need to at least do that.

One issue is that I've definitely seen the opposite situation as well: Combination of ISA feature macros being used to determine the CPU. That's also awful (and more error prone), and I wouldn't want to push users toward that either. There definitely are situations in which the presence/absence of a feature is not the relevant factor, but the performance of that feature, and so there may be different implementations of an algorithm depending on the identify of the targeted core (not just the ISA features).

Where is the best place to document this policy so people have a chance of understanding it going forward?

Where is the best place to document this policy so people have a chance of understanding it going forward?

Given the (apparent) amount of confusion here, I'd suggest a dedicated document in Clang's documentation that gives suggested best practices and warns about anti-patterns for detecting these kinds of things. We can write it reasonably generically and make it not too Clang-specific and citable for guidance even when folks are using GCC (or other compilers).

craig.topper planned changes to this revision.Oct 30 2017, 8:19 PM
craig.topper marked an inline comment as done.
craig.topper abandoned this revision.Nov 18 2017, 6:59 PM

All skylake-avx512 and cannonlake now set corei7 as of r318616. Abandoning this.