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

[craig.topper]

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.

[chandlerc]

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]

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]

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.

[craig.topper]

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]

Ping

[RKSimon]

lib/Basic/Targets/X86.cpp

837

defines

[craig.topper]

Fix Simon's comment

[chandlerc]

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.

[hfinkel]

In D38824#908461, @chandlerc wrote:

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).

[RKSimon]

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

[chandlerc]

In D38824#908540, @RKSimon wrote:

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]

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

lib/Basic/Targets/X86.cpp

case CK_Nocona:
     defineCPUMacros(Builder, "nocona");
     break;
   case CK_Core2:
   case CK_Penryn:
     defineCPUMacros(Builder, "core2");
     break;
   case CK_Bonnell:
     defineCPUMacros(Builder, "atom");
+    // gcc also defines 'bonnell', but we never have. See comment below.
     break;
   case CK_Silvermont:
     defineCPUMacros(Builder, "slm");
+    // gcc also defines 'silvermont', but we never have. See comment below.
     break;
   case CK_Goldmont:
     defineCPUMacros(Builder, "goldmont");
     break;
   case CK_Nehalem:
   case CK_Westmere:
+    defineCPUMacros(Builder, "corei7");
+    // gcc also defines 'nehalem', but we never have. See comment below.
+    break;
   case CK_SandyBridge:
   case CK_IvyBridge:
   case CK_Haswell:
   case CK_Broadwell:
   case CK_SkylakeClient:
-    // FIXME: Historically, we defined this legacy name, it would be nice to
-    // remove it at some point. We've never exposed fine-grained names for
-    // recent primary x86 CPUs, and we should keep it that way.
-    defineCPUMacros(Builder, "corei7");
-    break;
   case CK_SkylakeServer:
-    defineCPUMacros(Builder, "skx");
-    break;
   case CK_Cannonlake:
+  case CK_KNM:
+    // We don't want to define fine-grained macros for new CPUs going forward.
+    // While at the same time maintaining compatibility with gcc for the ones
+    // we have historically defined.
     break;
   case CK_KNL:
     defineCPUMacros(Builder, "knl");
     break;
-  case CK_KNM:
-    break;
   case CK_Lakemont:
     Builder.defineMacro("__tune_lakemont__");
     break;
   case CK_K6_2:
     Builder.defineMacro("__k6_2__");
     Builder.defineMacro("__tune_k6_2__");
     LLVM_FALLTHROUGH;
   case CK_K6_3:

test/Preprocessor/predefined-arch-macros.c

 // CHECK_COREI7_AVX_M32: #define __SSE2__ 1
 // CHECK_COREI7_AVX_M32: #define __SSE3__ 1
 // CHECK_COREI7_AVX_M32: #define __SSE4_1__ 1
 // CHECK_COREI7_AVX_M32: #define __SSE4_2__ 1
 // CHECK_COREI7_AVX_M32: #define __SSE__ 1
 // CHECK_COREI7_AVX_M32: #define __SSSE3__ 1
 // CHECK_COREI7_AVX_M32: #define __XSAVEOPT__ 1
 // CHECK_COREI7_AVX_M32: #define __XSAVE__ 1
-// CHECK_COREI7_AVX_M32: #define __corei7 1
-// CHECK_COREI7_AVX_M32: #define __corei7__ 1
 // CHECK_COREI7_AVX_M32: #define __i386 1
 // CHECK_COREI7_AVX_M32: #define __i386__ 1
-// CHECK_COREI7_AVX_M32: #define __tune_corei7__ 1
 // CHECK_COREI7_AVX_M32: #define i386 1
 // RUN: %clang -march=corei7-avx -m64 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_COREI7_AVX_M64
 // CHECK_COREI7_AVX_M64: #define __AES__ 1
 // CHECK_COREI7_AVX_M64: #define __AVX__ 1
 // CHECK_COREI7_AVX_M64: #define __MMX__ 1
 // CHECK_COREI7_AVX_M64: #define __PCLMUL__ 1
 // CHECK_COREI7_AVX_M64-NOT: __RDRND__
 // CHECK_COREI7_AVX_M64: #define __POPCNT__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE2_MATH__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE2__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE3__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE4_1__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE4_2__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE_MATH__ 1
 // CHECK_COREI7_AVX_M64: #define __SSE__ 1
 // CHECK_COREI7_AVX_M64: #define __SSSE3__ 1
 // CHECK_COREI7_AVX_M64: #define __XSAVEOPT__ 1
 // CHECK_COREI7_AVX_M64: #define __XSAVE__ 1
 // CHECK_COREI7_AVX_M64: #define __amd64 1
 // CHECK_COREI7_AVX_M64: #define __amd64__ 1
-// CHECK_COREI7_AVX_M64: #define __corei7 1
-// CHECK_COREI7_AVX_M64: #define __corei7__ 1
-// CHECK_COREI7_AVX_M64: #define __tune_corei7__ 1
 // CHECK_COREI7_AVX_M64: #define __x86_64 1
 // CHECK_COREI7_AVX_M64: #define __x86_64__ 1
 //
 // RUN: %clang -march=core-avx-i -m32 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_CORE_AVX_I_M32
 // CHECK_CORE_AVX_I_M32: #define __AES__ 1
 // CHECK_CORE_AVX_I_M32: #define __AVX__ 1
 // CHECK_CORE_AVX_I_M32: #define __F16C__ 1
 // CHECK_CORE_AVX_I_M32: #define __MMX__ 1
 // CHECK_CORE_AVX_I_M32: #define __PCLMUL__ 1
 // CHECK_CORE_AVX_I_M32: #define __RDRND__ 1
 // CHECK_CORE_AVX_I_M32: #define __SSE2__ 1
 // CHECK_CORE_AVX_I_M32: #define __SSE3__ 1
 // CHECK_CORE_AVX_I_M32: #define __SSE4_1__ 1
 // CHECK_CORE_AVX_I_M32: #define __SSE4_2__ 1
 // CHECK_CORE_AVX_I_M32: #define __SSE__ 1
 // CHECK_CORE_AVX_I_M32: #define __SSSE3__ 1
 // CHECK_CORE_AVX_I_M32: #define __XSAVEOPT__ 1
 // CHECK_CORE_AVX_I_M32: #define __XSAVE__ 1
-// CHECK_CORE_AVX_I_M32: #define __corei7 1
-// CHECK_CORE_AVX_I_M32: #define __corei7__ 1
 // CHECK_CORE_AVX_I_M32: #define __i386 1
 // CHECK_CORE_AVX_I_M32: #define __i386__ 1
-// CHECK_CORE_AVX_I_M32: #define __tune_corei7__ 1
 // CHECK_CORE_AVX_I_M32: #define i386 1
 // RUN: %clang -march=core-avx-i -m64 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_CORE_AVX_I_M64
 // CHECK_CORE_AVX_I_M64: #define __AES__ 1
 // CHECK_CORE_AVX_I_M64: #define __AVX__ 1
 // CHECK_CORE_AVX_I_M64: #define __F16C__ 1
 // CHECK_CORE_AVX_I_M64: #define __MMX__ 1
 // CHECK_CORE_AVX_I_M64: #define __PCLMUL__ 1
 // CHECK_CORE_AVX_I_M64: #define __RDRND__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE2_MATH__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE2__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE3__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE4_1__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE4_2__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE_MATH__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSE__ 1
 // CHECK_CORE_AVX_I_M64: #define __SSSE3__ 1
 // CHECK_CORE_AVX_I_M64: #define __XSAVEOPT__ 1
 // CHECK_CORE_AVX_I_M64: #define __XSAVE__ 1
 // CHECK_CORE_AVX_I_M64: #define __amd64 1
 // CHECK_CORE_AVX_I_M64: #define __amd64__ 1
-// CHECK_CORE_AVX_I_M64: #define __corei7 1
-// CHECK_CORE_AVX_I_M64: #define __corei7__ 1
-// CHECK_CORE_AVX_I_M64: #define __tune_corei7__ 1
 // CHECK_CORE_AVX_I_M64: #define __x86_64 1
 // CHECK_CORE_AVX_I_M64: #define __x86_64__ 1
 //
 // RUN: %clang -march=core-avx2 -m32 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_CORE_AVX2_M32
 // CHECK_CORE_AVX2_M32: #define __AES__ 1
 // CHECK_CORE_AVX2_M32: #define __AVX2__ 1

 // CHECK_CORE_AVX2_M32: #define __SSE2__ 1
 // CHECK_CORE_AVX2_M32: #define __SSE3__ 1
 // CHECK_CORE_AVX2_M32: #define __SSE4_1__ 1
 // CHECK_CORE_AVX2_M32: #define __SSE4_2__ 1
 // CHECK_CORE_AVX2_M32: #define __SSE__ 1
 // CHECK_CORE_AVX2_M32: #define __SSSE3__ 1
 // CHECK_CORE_AVX2_M32: #define __XSAVEOPT__ 1
 // CHECK_CORE_AVX2_M32: #define __XSAVE__ 1
-// CHECK_CORE_AVX2_M32: #define __corei7 1
-// CHECK_CORE_AVX2_M32: #define __corei7__ 1
 // CHECK_CORE_AVX2_M32: #define __i386 1
 // CHECK_CORE_AVX2_M32: #define __i386__ 1
-// CHECK_CORE_AVX2_M32: #define __tune_corei7__ 1
 // CHECK_CORE_AVX2_M32: #define i386 1
 // RUN: %clang -march=core-avx2 -m64 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_CORE_AVX2_M64
 // CHECK_CORE_AVX2_M64: #define __AES__ 1
 // CHECK_CORE_AVX2_M64: #define __AVX2__ 1
 // CHECK_CORE_AVX2_M64: #define __AVX__ 1
 // CHECK_CORE_AVX2_M64: #define __BMI2__ 1

 // CHECK_CORE_AVX2_M64: #define __SSE4_2__ 1
 // CHECK_CORE_AVX2_M64: #define __SSE_MATH__ 1
 // CHECK_CORE_AVX2_M64: #define __SSE__ 1
 // CHECK_CORE_AVX2_M64: #define __SSSE3__ 1
 // CHECK_CORE_AVX2_M64: #define __XSAVEOPT__ 1
 // CHECK_CORE_AVX2_M64: #define __XSAVE__ 1
 // CHECK_CORE_AVX2_M64: #define __amd64 1
 // CHECK_CORE_AVX2_M64: #define __amd64__ 1
-// CHECK_CORE_AVX2_M64: #define __corei7 1
-// CHECK_CORE_AVX2_M64: #define __corei7__ 1
-// CHECK_CORE_AVX2_M64: #define __tune_corei7__ 1
 // CHECK_CORE_AVX2_M64: #define __x86_64 1
 // CHECK_CORE_AVX2_M64: #define __x86_64__ 1
 //
 // RUN: %clang -march=broadwell -m32 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_BROADWELL_M32
 // CHECK_BROADWELL_M32: #define __ADX__ 1
 // CHECK_BROADWELL_M32: #define __AES__ 1

 // CHECK_BROADWELL_M32: #define __SSE2__ 1
 // CHECK_BROADWELL_M32: #define __SSE3__ 1
 // CHECK_BROADWELL_M32: #define __SSE4_1__ 1
 // CHECK_BROADWELL_M32: #define __SSE4_2__ 1
 // CHECK_BROADWELL_M32: #define __SSE__ 1
 // CHECK_BROADWELL_M32: #define __SSSE3__ 1
 // CHECK_BROADWELL_M32: #define __XSAVEOPT__ 1
 // CHECK_BROADWELL_M32: #define __XSAVE__ 1
-// CHECK_BROADWELL_M32: #define __corei7 1
-// CHECK_BROADWELL_M32: #define __corei7__ 1
 // CHECK_BROADWELL_M32: #define __i386 1
 // CHECK_BROADWELL_M32: #define __i386__ 1
-// CHECK_BROADWELL_M32: #define __tune_corei7__ 1
 // CHECK_BROADWELL_M32: #define i386 1
 // RUN: %clang -march=broadwell -m64 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_BROADWELL_M64
 // CHECK_BROADWELL_M64: #define __ADX__ 1
 // CHECK_BROADWELL_M64: #define __AES__ 1
 // CHECK_BROADWELL_M64: #define __AVX2__ 1
 // CHECK_BROADWELL_M64: #define __AVX__ 1

 // CHECK_BROADWELL_M64: #define __SSE4_2__ 1
 // CHECK_BROADWELL_M64: #define __SSE_MATH__ 1
 // CHECK_BROADWELL_M64: #define __SSE__ 1
 // CHECK_BROADWELL_M64: #define __SSSE3__ 1
 // CHECK_BROADWELL_M64: #define __XSAVEOPT__ 1
 // CHECK_BROADWELL_M64: #define __XSAVE__ 1
 // CHECK_BROADWELL_M64: #define __amd64 1
 // CHECK_BROADWELL_M64: #define __amd64__ 1
-// CHECK_BROADWELL_M64: #define __corei7 1
-// CHECK_BROADWELL_M64: #define __corei7__ 1
-// CHECK_BROADWELL_M64: #define __tune_corei7__ 1
 // CHECK_BROADWELL_M64: #define __x86_64 1
 // CHECK_BROADWELL_M64: #define __x86_64__ 1
 //
 // RUN: %clang -march=skylake -m32 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SKL_M32
 // CHECK_SKL_M32: #define __ADX__ 1
 // CHECK_SKL_M32: #define __AES__ 1

 // CHECK_SKX_M32: #define __SSE__ 1
 // CHECK_SKX_M32: #define __SSSE3__ 1
 // CHECK_SKX_M32: #define __XSAVEC__ 1
 // CHECK_SKX_M32: #define __XSAVEOPT__ 1
 // CHECK_SKX_M32: #define __XSAVES__ 1
 // CHECK_SKX_M32: #define __XSAVE__ 1
 // CHECK_SKX_M32: #define __i386 1
 // CHECK_SKX_M32: #define __i386__ 1
-// CHECK_SKX_M32: #define __skx 1
-// CHECK_SKX_M32: #define __skx__ 1
-// CHECK_SKX_M32: #define __tune_skx__ 1
 // CHECK_SKX_M32: #define i386 1
 
 // RUN: %clang -march=skylake-avx512 -m64 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SKX_M64
 // CHECK_SKX_M64: #define __AES__ 1
 // CHECK_SKX_M64: #define __AVX2__ 1
 // CHECK_SKX_M64: #define __AVX512BW__ 1

 // CHECK_SKX_M64: #define __SSE__ 1
 // CHECK_SKX_M64: #define __SSSE3__ 1
 // CHECK_SKX_M64: #define __XSAVEC__ 1
 // CHECK_SKX_M64: #define __XSAVEOPT__ 1
 // CHECK_SKX_M64: #define __XSAVES__ 1
 // CHECK_SKX_M64: #define __XSAVE__ 1
 // CHECK_SKX_M64: #define __amd64 1
 // CHECK_SKX_M64: #define __amd64__ 1
-// CHECK_SKX_M64: #define __skx 1
-// CHECK_SKX_M64: #define __skx__ 1
-// CHECK_SKX_M64: #define __tune_skx__ 1
 // CHECK_SKX_M64: #define __x86_64 1
 // CHECK_SKX_M64: #define __x86_64__ 1
 //
 // RUN: %clang -march=cannonlake -m32 -E -dM %s -o - 2>&1 \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_CNL_M32
 // CHECK_CNL_M32: #define __AES__ 1
 // CHECK_CNL_M32: #define __AVX2__ 1