Don't generate inline atomics for i386/i486

[wmi]

Summary

This is to fix the bug reported in https://bugs.llvm.org/show_bug.cgi?id=34347#c6

Currently, all MaxAtomicInlineWidth of x86-32 targets are set to 64. However, i386 doesn't support any cmpxchg related instructions. i486 only supports cmpxchg. So in this patch MaxAtomicInlineWidth is reset as follows.
For i486, the MaxAtomicInlineWidth should be 32 because it supports cmpxchg.
For i386, the MaxAtomicInlineWidth should be 0.
For others x86-32 cpu, the MaxAtomicInlineWidth should be 64 because of cmpxchg8b.

[efriedma]

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

lib/Basic/Targets/X86.h

472 ↗

(On Diff #130076)

What exactly does "CK_Generic" mean here? Under what circumstances does it show up? We should have a specific default for every supported target, if user doesn't specify a CPU with -march.

[craig.topper]

lib/Basic/Targets/X86.h

472 ↗

(On Diff #130076)

CK_Generic is the default for 32-bit mode with no -march. CK_x86_64 is the default for 64-bit mode with no march.

I'm not sure we can assume CK_Generic is 586 or better. We don't assume that in the preprocessor defines.

if (CPU >= CK_i486) {
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2");
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4");
}
if (CPU >= CK_i586)
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
if (HasCX16)
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16");

[craig.topper]

lib/Basic/Targets/X86.h

472 ↗

(On Diff #130076)

Maybe we don't default to "generic" anywhere. Its very platform specific. getX86TargetCPU in lib/Driver/ToolChains/Arch/X86.cpp contains the magic I think.

[wmi]

In D42154#977975, @efriedma wrote:

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

lib/Basic/Targets/X86.h

472 ↗

(On Diff #130076)

Like the command "clang -cc1 -triple=i686-linux-gnu", the cpu will be set to CK_Generic. I want to set the MaxAtomicInlineWidth of the default target to 64 (same as before) to minimize the churn. In the patch, MaxAtomicInlineWidth will only be changed if i386/i486 are explicitly specified.

[wmi]

lib/Basic/Targets/X86.h

472 ↗

(On Diff #130076)

In X86TargetInfo::getCPUKind (tools/clang/lib/Basic/Targets/X86.cpp), we default to "generic", for both 32 bits and 64 bits modes.

[craig.topper]

lib/Basic/Targets/X86.h

472 ↗

(On Diff #130076)

You're right. Without the driver we don't have a CPU string to use and end up defaulting to CK_Generic.

[rjmccall]

In D42154#977991, @wmi wrote:

In D42154#977975, @efriedma wrote:

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

I think Eli's point is that we do not currently support generating code for the 386 and 486 because there are other things in the x86 backend that assume that the target is at minimum a Pentium. If you're looking to support targeting those chips, you should look into that.

I don't have any objections to this patch.

[wmi]

In D42154#983840, @rjmccall wrote:

In D42154#977991, @wmi wrote:

In D42154#977975, @efriedma wrote:

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

I think Eli's point is that we do not currently support generating code for the 386 and 486 because there are other things in the x86 backend that assume that the target is at minimum a Pentium. If you're looking to support targeting those chips, you should look into that.

I am not looking to support those chips. Just because https://reviews.llvm.org/rL314145 changed the status of FreeBSD ports on 32 bits x86 as reported in https://bugs.llvm.org/show_bug.cgi?id=34347#c6, I want to provide a workaround and give them a relief.
This is also the intention of keeping MaxAtomicInlineWidth to be 64 for CK_Generic. In this way, it provides the minimum churn for current status -- otherwise at least a bunch of tests need to be fixed.

[rjmccall]

Well, my point is that the example in the linked bug is asking for 486 code-generation, which is apparently unsupported by LLVM.

Anyway, it's not a good reason to hold up this patch, since the way to support things is to gradually fix the bugs with them. LGTM.

cfe/trunk/lib/Basic/Targets/X86.h

class LLVM_LIBRARY_VISIBILITY X86TargetInfo : public TargetInfo {
   bool HasCLFLUSHOPT = false;
   bool HasCLWB = false;
   bool HasMOVBE = false;
   bool HasPREFETCHWT1 = false;
   bool HasRDPID = false;
   bool HasRetpoline = false;
   bool HasRetpolineExternalThunk = false;
 
+protected:
   /// \brief Enumeration of all of the X86 CPUs supported by Clang.
   ///
   /// Each enumeration represents a particular CPU supported by Clang. These
   /// loosely correspond to the options passed to '-march' or '-mtune' flags.
   enum CPUKind {
     CK_Generic,
 #define PROC(ENUM, STRING, IS64BIT) CK_##ENUM,
 #include "clang/Basic/X86Target.def"
X86_32TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
     RegParmMax = 3;
 
     // Use fpret for all types.
     RealTypeUsesObjCFPRet =
         ((1 << TargetInfo::Float) | (1 << TargetInfo::Double) |
          (1 << TargetInfo::LongDouble));
 
     // x86-32 has atomics up to 8 bytes
-    // FIXME: Check that we actually have cmpxchg8b before setting
-    // MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
+    CPUKind Kind = getCPUKind(Opts.CPU);
+    if (Kind >= CK_i586 || Kind == CK_Generic)
       MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+    else if (Kind >= CK_i486)
+      MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 32;
   }
 
   BuiltinVaListKind getBuiltinVaListKind() const override {
     return TargetInfo::CharPtrBuiltinVaList;
   }
 
   int getEHDataRegisterNumber(unsigned RegNo) const override {
     if (RegNo == 0)

cfe/trunk/test/CodeGenCXX/atomic-inline.cpp

 // RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - -triple=x86_64-linux-gnu | FileCheck %s
 // RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - -triple=x86_64-linux-gnu -target-cpu core2 | FileCheck %s --check-prefix=CORE2
-// Check the atomic code generation for cpu targets w/wo cx16 support.
+// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - -triple=i386-linux-gnu -target-cpu i386 | FileCheck %s --check-prefix=I386
+// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - -triple=i386-linux-gnu -target-cpu i486 | FileCheck %s --check-prefix=I486
+// Check the atomic code generation for cpu targets w/wo cx, cx8 and cx16 support.
+
+struct alignas(4) AM4 {
+  short f1, f2;
+};
+AM4 m4;
+AM4 load4() {
+  AM4 am;
+  // CHECK-LABEL: @_Z5load4v
+  // CHECK: load atomic i32, {{.*}} monotonic
+  // CORE2-LABEL: @_Z5load4v
+  // CORE2: load atomic i32, {{.*}} monotonic
+  // I386-LABEL: @_Z5load4v
+  // I386: call i32 @__atomic_load_4
+  // I486-LABEL: @_Z5load4v
+  // I486: load atomic i32, {{.*}} monotonic
+  __atomic_load(&m4, &am, 0);
+  return am;
+}
+
+AM4 s4;
+void store4() {
+  // CHECK-LABEL: @_Z6store4v
+  // CHECK: store atomic i32 {{.*}} monotonic
+  // CORE2-LABEL: @_Z6store4v
+  // CORE2: store atomic i32 {{.*}} monotonic
+  // I386-LABEL: @_Z6store4v
+  // I386: call void @__atomic_store_4
+  // I486-LABEL: @_Z6store4v
+  // I486: store atomic i32 {{.*}} monotonic
+  __atomic_store(&m4, &s4, 0);
+}
+
+bool cmpxchg4() {
+  AM4 am;
+  // CHECK-LABEL: @_Z8cmpxchg4v
+  // CHECK: cmpxchg i32* {{.*}} monotonic
+  // CORE2-LABEL: @_Z8cmpxchg4v
+  // CORE2: cmpxchg i32* {{.*}} monotonic
+  // I386-LABEL: @_Z8cmpxchg4v
+  // I386: call zeroext i1 @__atomic_compare_exchange_4
+  // I486-LABEL: @_Z8cmpxchg4v
+  // I486: cmpxchg i32* {{.*}} monotonic
+  return __atomic_compare_exchange(&m4, &s4, &am, 0, 0, 0);
+}
 
 struct alignas(8) AM8 {
   int f1, f2;
 };
 AM8 m8;
 AM8 load8() {
   AM8 am;
   // CHECK-LABEL: @_Z5load8v
   // CHECK: load atomic i64, {{.*}} monotonic
   // CORE2-LABEL: @_Z5load8v
   // CORE2: load atomic i64, {{.*}} monotonic
+  // I386-LABEL: @_Z5load8v
+  // I386: call i64 @__atomic_load_8
+  // I486-LABEL: @_Z5load8v
+  // I486: call i64 @__atomic_load_8
   __atomic_load(&m8, &am, 0);
   return am;
 }
 
 AM8 s8;
 void store8() {
   // CHECK-LABEL: @_Z6store8v
   // CHECK: store atomic i64 {{.*}} monotonic
   // CORE2-LABEL: @_Z6store8v
   // CORE2: store atomic i64 {{.*}} monotonic
+  // I386-LABEL: @_Z6store8v
+  // I386: call void @__atomic_store_8
+  // I486-LABEL: @_Z6store8v
+  // I486: call void @__atomic_store_8
   __atomic_store(&m8, &s8, 0);
 }
 
 bool cmpxchg8() {
   AM8 am;
   // CHECK-LABEL: @_Z8cmpxchg8v
   // CHECK: cmpxchg i64* {{.*}} monotonic
   // CORE2-LABEL: @_Z8cmpxchg8v
   // CORE2: cmpxchg i64* {{.*}} monotonic
+  // I386-LABEL: @_Z8cmpxchg8v
+  // I386: call zeroext i1 @__atomic_compare_exchange_8
+  // I486-LABEL: @_Z8cmpxchg8v
+  // I486: call zeroext i1 @__atomic_compare_exchange_8
   return __atomic_compare_exchange(&m8, &s8, &am, 0, 0, 0);
 }
 
 struct alignas(16) AM16 {
   long f1, f2;
 };
 
 AM16 m16;

 bool cmpxchg16() {
   AM16 am;
   // CHECK-LABEL: @_Z9cmpxchg16v
   // CHECK: call zeroext i1 @__atomic_compare_exchange
   // CORE2-LABEL: @_Z9cmpxchg16v
   // CORE2: cmpxchg i128* {{.*}} monotonic
   return __atomic_compare_exchange(&m16, &s16, &am, 0, 0, 0);
 }
-