[X86] Only define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 in 64-bit mode.

[craig.topper]

Summary

This define should correspond to CMPXCHG16B being available which requires 64-bit mode.

I checked and gcc also seems to only define this in 64-bit mode.

[jfb]

I don't think this is quite right: CX16 is literally "I have cmpxchg16b".

In clang/lib/Basic/Targets/X86.h we do:

	  void setMaxAtomicWidth() override {
	    if (hasFeature("cx16"))
	      MaxAtomicInlineWidth = 128;
	  }

Your change makes it inconsistent.

We also have HasCX16 which should be kept consistent.

The problem seems to be that cx16 can be set for 32-bit targets.

[craig.topper]

Isn’t that setMaxAtomicWidth in the x86-64 derived class?

As far as preventing “cx16” from being set in 32-bit mode, we’ll need to check the behavior of CPUID in 32-bit mode or -march=native might still end up setting it.

[jfb]

In D59287#1427485, @craig.topper wrote:

Isn’t that setMaxAtomicWidth in the x86-64 derived class?

Right you are!

As far as preventing “cx16” from being set in 32-bit mode, we’ll need to check the behavior of CPUID in 32-bit mode or -march=native might still end up setting it.

What I want to make sure is that your fix to the macro (which seems correct!) doesn't diverge from what the rest LLVM ends up doing with atomics. i.e. the macro says "no cmpxchg16b" but somehow codegen does generate it.

[craig.topper]

Most if not all of the test cases in test/CodeGen/X86/atomic128.ll fail with a fatal error if you run it in 32-bit mode with -mattr=+cx16 Looks like the backend is also bad at checking 64 bit mode.

[jfb]

In D59287#1427654, @craig.topper wrote:

Most if not all of the test cases in test/CodeGen/X86/atomic128.ll fail with a fatal error if you run it in 32-bit mode with -mattr=+cx16 Looks like the backend is also bad at checking 64 bit mode.

So you're saying we're consistent, but in a bad way? It seems like an i386 target just shouldn't be allowed to set cx16, no?

[craig.topper]

Is this ok with the backend fixed? Or do you want me factor this into HasCX16 which I think is only used by the defineMacro and the return for hasFeature("cx16")? And I think hasFeature("cx16") is only used by that getMaxAtomicWidth() code which is only called on 64 bit.

Or we could maybe ignore "cx16" in setFeatureEnabled on 32 bit targets? But I think that would break always_inline on a target attribute with cx16 in 32 bit mode which gcc does allow. https://godbolt.org/z/TW985s

[jfb]

In D59287#1427945, @craig.topper wrote:

Is this ok with the backend fixed?

This is definitely better.

Or do you want me factor this into HasCX16 which I think is only used by the defineMacro and the return for hasFeature("cx16")? And I think hasFeature("cx16") is only used by that getMaxAtomicWidth() code which is only called on 64 bit.

Or we could maybe ignore "cx16" in setFeatureEnabled on 32 bit targets? But I think that would break always_inline on a target attribute with cx16 in 32 bit mode which gcc does allow. https://godbolt.org/z/TW985s

I'm not sure. Does clang ever error out when you have inconsistent platform features and arch on the command line? That seems like what we should be doing here, no?
Because your change just hides a mistake, and clang is usually the only place where we catch mistakes (the rest of LLVM can't diagnose).

[craig.topper]

I think the only error we have for X86 is trying to use a -march for a cpu that only supports 32 bit but compiling 64 bit code.

I dont' think we can error for -mcx16 on a 32-bit target. For -march=native, the driver will call getHostCPUFeatures and get a list of features. As far as I can tell CPUID will report cx16 is supported even in 32-bit mode if the host CPU supports it in 64-bit mode. The driver will pass the list of features returned by getHostCPUFeatures onto the command line of the cc1 invocation. Those command line options will then be fed to initFeatureMap/setFeatureEnabled. We don't have the information to distinquish that from the user passing -mcx16. -mcx16 is intercepted by the driver and passed to cc1 in a similar way. I don't think we can filter cx16 from getHostCPUFeatures in 32-bit mode since the host might be running in 32-bit mode, but we could be passing -m64 to the compiler. So I don't think getHostCPUFeatures can look at the current operating mode of the CPU.

I guess we could error for -mcx16 in the driver itself. But due to march native we'd still have to protect the code in cc1. Unless we also had the driver filter the output of getHostCPUFeatures before passing to cc1. Which if we did that then we probably could error from initFeatureMap/setFeatureEnabled.

But gcc doesn't error for any use of cx16 in 32-bit mode so might be bad for compatibility anyway?

[jfb]

OK, that seems unfortunate but unlikely and consistency terrible with GCC. Let's do it.

[RKSimon]

I agree that its not great, but I don't think we're going to do much better - and matching gcc behaviours does make sense here.

lib/Basic/Targets/X86.cpp

void X86TargetInfo::getTargetDefines(const LangOptions &Opts,
 
   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)
+  if (HasCX16 && getTriple().getArch() == llvm::Triple::x86_64)
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16");
 
   if (HasFloat128)
     Builder.defineMacro("__SIZEOF_FLOAT128__", "16");
 }
 
 bool X86TargetInfo::isValidFeatureName(StringRef Name) const {
   return llvm::StringSwitch<bool>(Name)

test/Preprocessor/x86_target_features.c

 // RUN: %clang -target i386-unknown-unknown -march=atom -mtbm -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=TBM %s
 
 // TBM: #define __TBM__ 1
 
 // RUN: %clang -target i386-unknown-unknown -march=bdver2 -mno-tbm -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=NOTBM %s
 
 // NOTBM-NOT: #define __TBM__ 1
 
-// RUN: %clang -target i386-unknown-unknown -march=pentiumpro -mcx16 -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=MCX16 %s
+// RUN: %clang -target i386-unknown-unknown -march=pentiumpro -mcx16 -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=MCX16-32 %s
 
-// MCX16: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 1
+// MCX16-32-NOT: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 1
+
+// RUN: %clang -target x86_64-unknown-unknown -march=x86-64 -mcx16 -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=MCX16-64 %s
+
+// MCX16-64: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 1
 
 // RUN: %clang -target i386-unknown-unknown -march=atom -mprfchw -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=PRFCHW %s
 
 // PRFCHW: #define __PRFCHW__ 1
 
 // RUN: %clang -target i386-unknown-unknown -march=btver2 -mno-prfchw -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=NOPRFCHW %s
 
 // NOPRFCHW-NOT: #define __PRFCHW__ 1