[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Ryan Prichard via Phabricator via cfe-commits]

rprichard added subscribers: craig.topper, rprichard.
rprichard added a comment.
Herald added a project: All.

Maybe this change is obsolete now that D59566  
is merged?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D29542/new/

https://reviews.llvm.org/D29542

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny marked 5 inline comments as done.
mgorny added a comment.

A gentle ping.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: test/CodeGen/atomic-ops.c:1
-// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map 
-triple=i686-apple-darwin9 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map 
-triple=i686-apple-darwin9 -target-cpu i686 | FileCheck %s
 // REQUIRES: x86-registered-target

hans wrote:
> Naive question: why is the i686- part of the triple not sufficient here; why 
> is -target-cpu needed?
It's because triple is not really meaningful on most of the systems (e.g. many 
Linux distros use i386, *BSD use i486), and the default CPU logic is applied in 
the Driver, while cc1 is called directly here.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Hans Wennborg via Phabricator via cfe-commits]

hans added a reviewer: jyknight.
hans added a comment.

+jyknight, who had a similar patch in http://reviews.llvm.org/D17933 (see also 
r291477 and PR31864)




Comment at: test/CodeGen/atomic-ops.c:1
-// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map 
-triple=i686-apple-darwin9 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map 
-triple=i686-apple-darwin9 -target-cpu i686 | FileCheck %s
 // REQUIRES: x86-registered-target

Naive question: why is the i686- part of the triple not sufficient here; why is 
-target-cpu needed?


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Le gentle ping.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 87311.
mgorny added a comment.

Removed the i386 branch. Now the i486+ are used unconditionally.


https://reviews.llvm.org/D29542

Files:
  lib/Basic/Targets.cpp
  test/CodeGen/atomic-ops.c
  test/CodeGen/ms-volatile.c
  test/CodeGenCXX/atomicinit.cpp
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -target-cpu i686 -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i486-linux-gnu -target-cpu i486 -std=c11
 
 // Basic parsing/Sema tests for __c11_atomic_*
 
@@ -14,22 +15,34 @@
 _Static_assert(__GCC_ATOMIC_SHORT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_INT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_LONG_LOCK_FREE == 2, "");
+#if defined(__i486__)
+_Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 1, "");
+#else
 _Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 2, "");
+#endif
 _Static_assert(__GCC_ATOMIC_POINTER_LOCK_FREE == 2, "");
 
 _Static_assert(__c11_atomic_is_lock_free(1), "");
 _Static_assert(__c11_atomic_is_lock_free(2), "");
 _Static_assert(__c11_atomic_is_lock_free(3), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(4), "");
+#if defined(__i486__)
+_Static_assert(__c11_atomic_is_lock_free(8), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__c11_atomic_is_lock_free(8), "");
+#endif
 _Static_assert(__c11_atomic_is_lock_free(16), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(17), ""); // expected-error {{not an integral constant expression}}
 
 _Static_assert(__atomic_is_lock_free(1, 0), "");
 _Static_assert(__atomic_is_lock_free(2, 0), "");
 _Static_assert(__atomic_is_lock_free(3, 0), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__atomic_is_lock_free(4, 0), "");
+#if defined(__i486__)
+_Static_assert(__atomic_is_lock_free(8, 0), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__atomic_is_lock_free(8, 0), "");
+#endif
 _Static_assert(__atomic_is_lock_free(16, 0), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__atomic_is_lock_free(17, 0), ""); // expected-error {{not an integral constant expression}}
 
@@ -56,13 +69,21 @@
 _Static_assert(__atomic_is_lock_free(4, ), "");
 _Static_assert(__atomic_is_lock_free(4, ), "");
 _Static_assert(__atomic_is_lock_free(8, ), ""); // expected-error {{not an integral constant expression}}
+#if defined(__i486__)
+_Static_assert(__atomic_is_lock_free(8, ), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__atomic_is_lock_free(8, ), "");
+#endif
 
 _Static_assert(__atomic_always_lock_free(1, 0), "");
 _Static_assert(__atomic_always_lock_free(2, 0), "");
 _Static_assert(!__atomic_always_lock_free(3, 0), "");
 _Static_assert(__atomic_always_lock_free(4, 0), "");
+#if defined(__i486__)
+_Static_assert(!__atomic_always_lock_free(8, 0), "");
+#else
 _Static_assert(__atomic_always_lock_free(8, 0), "");
+#endif
 _Static_assert(!__atomic_always_lock_free(16, 0), "");
 _Static_assert(!__atomic_always_lock_free(17, 0), "");
 
@@ -79,7 +100,11 @@
 _Static_assert(__atomic_always_lock_free(4, ), "");
 _Static_assert(__atomic_always_lock_free(4, ), "");
 _Static_assert(!__atomic_always_lock_free(8, ), "");
+#if defined(__i486__)
+_Static_assert(!__atomic_always_lock_free(8, ), "");
+#else
 _Static_assert(__atomic_always_lock_free(8, ), "");
+#endif
 
 #define _AS1 __attribute__((address_space(1)))
 #define _AS2 __attribute__((address_space(2)))
Index: test/CodeGenCXX/atomicinit.cpp
===
--- test/CodeGenCXX/atomicinit.cpp
+++ test/CodeGenCXX/atomicinit.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -emit-llvm -O1 -o - -triple=i686-apple-darwin9 -std=c++11 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -O1 -o - -triple=i686-apple-darwin9 -target-cpu i686 -std=c++11 | FileCheck %s
 
 // CHECK-DAG: @PR22043 = local_unnamed_addr global i32 0, align 4
 typedef _Atomic(int) AtomicInt;
Index: test/CodeGen/ms-volatile.c
===
--- test/CodeGen/ms-volatile.c
+++ test/CodeGen/ms-volatile.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i386-pc-win32 -fms-extensions -emit-llvm -fms-volatile -o - < %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -target-cpu i686 -fms-extensions -emit-llvm -fms-volatile -o - < %s | FileCheck %s
 struct foo {
   volatile int x;
 };
Index: test/CodeGen/atomic-ops.c
===
--- 

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I think you're running into https://llvm.org/bugs/show_bug.cgi?id=31620 .


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Dimitry Andric via Phabricator via cfe-commits]

dim added a comment.

There's still something strange here.  If I compile the following on 
i386-freebsd12, with `clang -march=i486 -O2 -S`:

  _Atomic(long long) ll;
  
  void f(void)
  {
++ll;
  }

the result is:

.globl  f
.p2align4, 0x90
.type   f,@function
  f:  # @f
  # BB#0: # %entry
pushl   %ebp
movl%esp, %ebp
pushl   %ebx
movlll+4, %edx
movlll, %eax
.p2align4, 0x90
  .LBB0_1:# %atomicrmw.start
  # =>This Inner Loop Header: Depth=1
movl%eax, %ebx
addl$1, %ebx
movl%edx, %ecx
adcl$0, %ecx
lockcmpxchg8b   ll
jne .LBB0_1
  # BB#2: # %atomicrmw.end
popl%ebx
popl%ebp
retl
  .Lfunc_end0:
.size   f, .Lfunc_end0-f
  
.type   ll,@object  # @ll
.comm   ll,8,4

So what gives?  It's still inserting a `cmpxchg8b`!  AFAIK it should now insert 
a call to `__atomic_fetch_add_8` instead.

Note that this changes if you use C++ atomics, e.g.:

  #include 
  
  void f(std::atomic& x)
  {
++x;
  }

compiles to:

.globl  _Z1fRNSt3__16atomicIxEE
.p2align4, 0x90
.type   _Z1fRNSt3__16atomicIxEE,@function
  _Z1fRNSt3__16atomicIxEE:# @_Z1fRNSt3__16atomicIxEE
  .Lfunc_begin0:
.cfi_sections .debug_frame
.cfi_startproc
.cfi_personality 0, __gxx_personality_v0
.cfi_lsda 0, .Lexception0
  # BB#0: # %entry
pushl   %ebp
movl%esp, %ebp
subl$16, %esp
movl8(%ebp), %eax
  .Ltmp0:
movl%eax, (%esp)
movl$5, 12(%esp)
movl$0, 8(%esp)
movl$1, 4(%esp)
calll   __atomic_fetch_add_8
  .Ltmp1:
  # BB#1: # 
%_ZNSt3__113__atomic_baseIxLb1EEppEv.exit
addl$16, %esp
popl%ebp
retl
  .LBB0_2:# %lpad.i.i
  .Ltmp2:
movl%eax, (%esp)
calll   __cxa_call_unexpected
subl$4, %esp
  .Lfunc_end0:
.size   _Z1fRNSt3__16atomicIxEE, .Lfunc_end0-_Z1fRNSt3__16atomicIxEE
.cfi_endproc


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

> Well, the thing is, we don't call HostTarget->setCPU() before this function. 
> We just call AllocateTarget(), and it does not set the CPU.

Ah, got it.  LGTM for the nvptx changes.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: lib/Basic/Targets.cpp:4244
+} else // allow locked atomics up to 4 bytes
+  MaxAtomicPromoteWidth = 32;
+  }

ahatanak wrote:
> mgorny wrote:
> > dim wrote:
> > > Are you purposefully not setting `MaxAtomicInlineWidth` here?  (It seems 
> > > from `TargetInfo` that the default value is zero.)
> > > 
> > Yes. I've based this on what's done for ARM. Unless I misunderstood 
> > something, this means that on 'plain' i386 there is no inline atomics 
> > support but we still want to do atomics-via-locking up to 32-bit types. I'm 
> > not sure about 32/64 here to match i486.
> If there isn't a test case for plain i386, is it possible to add one (perhaps 
> in test/Sema/atomic-ops.c)?
I could do that. However, @joerg suggested dropping i386 branch entirely, and 
assuming i486+.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/Basic/Targets.cpp:4244
+} else // allow locked atomics up to 4 bytes
+  MaxAtomicPromoteWidth = 32;
+  }

mgorny wrote:
> dim wrote:
> > Are you purposefully not setting `MaxAtomicInlineWidth` here?  (It seems 
> > from `TargetInfo` that the default value is zero.)
> > 
> Yes. I've based this on what's done for ARM. Unless I misunderstood 
> something, this means that on 'plain' i386 there is no inline atomics support 
> but we still want to do atomics-via-locking up to 32-bit types. I'm not sure 
> about 32/64 here to match i486.
If there isn't a test case for plain i386, is it possible to add one (perhaps 
in test/Sema/atomic-ops.c)?


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

Generic should imply i486+. I don't think any general purpose system supports 
i386 at this point, simply because it has an annoying number of bugs in 
critical components. The i486 (esp. the non-crippled ones) are reasonable easy 
to support and there are still people around with hardware, esp. clones.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

In https://reviews.llvm.org/D29542#667814, @joerg wrote:

> At this point, I don't think there is any use on pretending that 
> i386-as-default makes sense. So I would request that the i386 case should be 
> made explicit or just dropped, with a preference for the latter.


By the former, do you mean making `CK_Generic` imply i486+ or i586+? What about 
line ~3947 where the same conditions are used to control other definitions? 
Should they be changed too?


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: lib/Basic/Targets.cpp:1808
+if (HostTriple.getArch() == llvm::Triple::x86)
+  HostTarget->setCPU("i586");
+

jlebar wrote:
> mgorny wrote:
> > jlebar wrote:
> > > Okay, is this still needed now?
> > Yes. I've specifically tested with it commented out, and the CPU gets 
> > initiated to generic (=no inline atomics) then.
> Yes, but is that a bug?  Does that break the test?
> 
> I thought the problem we were trying to solve here was that CUDA host and 
> device builds did not define the same macros.  And I thought that setCPU 
> modified the values for MaxAtomicInlineWidth and MaxAtomicPromoteWidth.  
> Moreover I thought that we called HostTarget->setCPU before calling this 
> function.
> 
> If all of those things are true, I don't see what problem we're solving by 
> calling HostTarget->setCPU("i586") here.
Well, the thing is, we don't call `HostTarget->setCPU()` before this function. 
We just call `AllocateTarget()`, and it does not set the CPU.

Normally the CPU is set in Driver, based on `-march` etc. if provided, with 
fallback to platform-specific defaults. In the case of host-side CUDA build, 
the Driver sets x86-specific CPU. While the defaults differ per platform, for 
all platforms supporting CUDA it's i586+.

Now, for the target-side, the Driver creates NVPTX target, and sets 
NVPTX-specific CPU. The `HostTarget` instance is only created within 
`NVPTXTargetInfo`, and so we need to `setCPU()` explicitly. Since we can 
reliably assume that the host-side will be i586+, we use `i586` here.

So far this didn't matter since all atomic properties were defined statically. 
However, this patch changes them to adjust to the CPU used, and so if the 
`X8632TargetInfo` instance is allocated without an explicit `setCPU()` call, it 
defaults to generic x86 (= no inline atomics available) which is different from 
the host platform default. As a result, different macros are defined and the 
test fails.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added a comment.

At this point, I don't think there is any use on pretending that 
i386-as-default makes sense. So I would request that the i386 case should be 
made explicit or just dropped, with a preference for the latter.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Justin Lebar via Phabricator via cfe-commits]

jlebar added inline comments.



Comment at: lib/Basic/Targets.cpp:1808
+if (HostTriple.getArch() == llvm::Triple::x86)
+  HostTarget->setCPU("i586");
+

mgorny wrote:
> jlebar wrote:
> > Okay, is this still needed now?
> Yes. I've specifically tested with it commented out, and the CPU gets 
> initiated to generic (=no inline atomics) then.
Yes, but is that a bug?  Does that break the test?

I thought the problem we were trying to solve here was that CUDA host and 
device builds did not define the same macros.  And I thought that setCPU 
modified the values for MaxAtomicInlineWidth and MaxAtomicPromoteWidth.  
Moreover I thought that we called HostTarget->setCPU before calling this 
function.

If all of those things are true, I don't see what problem we're solving by 
calling HostTarget->setCPU("i586") here.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: lib/Basic/Targets.cpp:1808
+if (HostTriple.getArch() == llvm::Triple::x86)
+  HostTarget->setCPU("i586");
+

jlebar wrote:
> Okay, is this still needed now?
Yes. I've specifically tested with it commented out, and the CPU gets initiated 
to generic (=no inline atomics) then.


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Justin Lebar via Phabricator via cfe-commits]

jlebar added inline comments.



Comment at: lib/Basic/Targets.cpp:1808
+if (HostTriple.getArch() == llvm::Triple::x86)
+  HostTarget->setCPU("i586");
+

Okay, is this still needed now?


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Justin Lebar via Phabricator via cfe-commits]

jlebar added inline comments.



Comment at: lib/Basic/Targets.cpp:1784
+  // atomics. This matches the defaults for the systems using CUDA.
+  // TODO: pass the host target CPU
+  if (HostTriple.getArch() == llvm::Triple::x86)

Someone else is calling setCPU on the HostTarget.  If they're calling it after 
we call it here, this is obviously not going to work.  Since it does work, I 
presume they are calling it before we get here.  In which case, can we not just 
set `MaxAtomicPromoteWidth=HostTarget->MaxAtomicPromoteWidth` right after we 
set `MaxAtomicInlineWidth = HostTarget->getMaxAtomicInlineWidth();` below?

(Note that as written definitely isn't right because it assumes that HostTarget 
is non-null.)


https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 87134.
mgorny edited the summary of this revision.
mgorny added a comment.

Ok, this CUDA fix should be reasonable, i think. It simply assumes i586+ (i.e. 
all inline atomics enabled) for CUDA target builds. I seriously doubt it's 
technically possible that anyone will ever use CUDA on https://reviews.llvm.org/D29542

Files:
  lib/Basic/Targets.cpp
  test/CodeGen/atomic-ops.c
  test/CodeGen/ms-volatile.c
  test/CodeGenCXX/atomicinit.cpp
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -target-cpu i686 -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i486-linux-gnu -target-cpu i486 -std=c11
 
 // Basic parsing/Sema tests for __c11_atomic_*
 
@@ -14,22 +15,34 @@
 _Static_assert(__GCC_ATOMIC_SHORT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_INT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_LONG_LOCK_FREE == 2, "");
+#if defined(__i486__)
+_Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 1, "");
+#else
 _Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 2, "");
+#endif
 _Static_assert(__GCC_ATOMIC_POINTER_LOCK_FREE == 2, "");
 
 _Static_assert(__c11_atomic_is_lock_free(1), "");
 _Static_assert(__c11_atomic_is_lock_free(2), "");
 _Static_assert(__c11_atomic_is_lock_free(3), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(4), "");
+#if defined(__i486__)
+_Static_assert(__c11_atomic_is_lock_free(8), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__c11_atomic_is_lock_free(8), "");
+#endif
 _Static_assert(__c11_atomic_is_lock_free(16), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(17), ""); // expected-error {{not an integral constant expression}}
 
 _Static_assert(__atomic_is_lock_free(1, 0), "");
 _Static_assert(__atomic_is_lock_free(2, 0), "");
 _Static_assert(__atomic_is_lock_free(3, 0), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__atomic_is_lock_free(4, 0), "");
+#if defined(__i486__)
+_Static_assert(__atomic_is_lock_free(8, 0), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__atomic_is_lock_free(8, 0), "");
+#endif
 _Static_assert(__atomic_is_lock_free(16, 0), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__atomic_is_lock_free(17, 0), ""); // expected-error {{not an integral constant expression}}
 
@@ -56,13 +69,21 @@
 _Static_assert(__atomic_is_lock_free(4, ), "");
 _Static_assert(__atomic_is_lock_free(4, ), "");
 _Static_assert(__atomic_is_lock_free(8, ), ""); // expected-error {{not an integral constant expression}}
+#if defined(__i486__)
+_Static_assert(__atomic_is_lock_free(8, ), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__atomic_is_lock_free(8, ), "");
+#endif
 
 _Static_assert(__atomic_always_lock_free(1, 0), "");
 _Static_assert(__atomic_always_lock_free(2, 0), "");
 _Static_assert(!__atomic_always_lock_free(3, 0), "");
 _Static_assert(__atomic_always_lock_free(4, 0), "");
+#if defined(__i486__)
+_Static_assert(!__atomic_always_lock_free(8, 0), "");
+#else
 _Static_assert(__atomic_always_lock_free(8, 0), "");
+#endif
 _Static_assert(!__atomic_always_lock_free(16, 0), "");
 _Static_assert(!__atomic_always_lock_free(17, 0), "");
 
@@ -79,7 +100,11 @@
 _Static_assert(__atomic_always_lock_free(4, ), "");
 _Static_assert(__atomic_always_lock_free(4, ), "");
 _Static_assert(!__atomic_always_lock_free(8, ), "");
+#if defined(__i486__)
+_Static_assert(!__atomic_always_lock_free(8, ), "");
+#else
 _Static_assert(__atomic_always_lock_free(8, ), "");
+#endif
 
 #define _AS1 __attribute__((address_space(1)))
 #define _AS2 __attribute__((address_space(2)))
Index: test/CodeGenCXX/atomicinit.cpp
===
--- test/CodeGenCXX/atomicinit.cpp
+++ test/CodeGenCXX/atomicinit.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -emit-llvm -O1 -o - -triple=i686-apple-darwin9 -std=c++11 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -O1 -o - -triple=i686-apple-darwin9 -target-cpu i686 -std=c++11 | FileCheck %s
 
 // CHECK-DAG: @PR22043 = local_unnamed_addr global i32 0, align 4
 typedef _Atomic(int) AtomicInt;
Index: test/CodeGen/ms-volatile.c
===
--- test/CodeGen/ms-volatile.c
+++ test/CodeGen/ms-volatile.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple i386-pc-win32 -fms-extensions -emit-llvm -fms-volatile -o - < %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-pc-win32 -target-cpu i686 -fms-extensions -emit-llvm -fms-volatile 

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

In https://reviews.llvm.org/D29542#666831, @jlebar wrote:

> > Could someone help me figure out what is the cause and correct solution to 
> > that failure? @jlebar?
>
> You can see in NVPTXTargetInfo that we read properties from the host 
> targetinfo so that we export the same macros.  The problem here seems to be 
> that we're mutating the x86 targetinfo after the nvptx targetinfo reads its 
> properties.
>
> Does that give you enough context to fix the problem?


Thanks. I'll try to find a reasonably sane solution ;-).




Comment at: lib/Basic/Targets.cpp:4244
+} else // allow locked atomics up to 4 bytes
+  MaxAtomicPromoteWidth = 32;
+  }

dim wrote:
> Are you purposefully not setting `MaxAtomicInlineWidth` here?  (It seems from 
> `TargetInfo` that the default value is zero.)
> 
Yes. I've based this on what's done for ARM. Unless I misunderstood something, 
this means that on 'plain' i386 there is no inline atomics support but we still 
want to do atomics-via-locking up to 32-bit types. I'm not sure about 32/64 
here to match i486.



Comment at: lib/Basic/Targets.cpp:4265
 
-// x86-32 has atomics up to 8 bytes
-// FIXME: Check that we actually have cmpxchg8b before setting
-// MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
-MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+setAtomic();
   }

dim wrote:
> As far as I can see, in the constructor this call is _always_ made with `CPU` 
> set to `CK_Generic`, i.e. zero.  Therefore, the "allow locked atomics up to 4 
> bytes" path in `setAtomic` is always chosen.  Maybe it is clearer to just 
> initialize `MaxAtomicPromoteWidth` to 32 directly here, instead?
> 
Well, I just copied the idea from ARM. I thought of it more like 'make sure it 
is initialized to some value, possibly update it later when setting CPU'. I'm 
fine either way.


Repository:
  rL LLVM

https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Dimitry Andric via Phabricator via cfe-commits]

dim added inline comments.



Comment at: lib/Basic/Targets.cpp:4244
+} else // allow locked atomics up to 4 bytes
+  MaxAtomicPromoteWidth = 32;
+  }

Are you purposefully not setting `MaxAtomicInlineWidth` here?  (It seems from 
`TargetInfo` that the default value is zero.)




Comment at: lib/Basic/Targets.cpp:4265
 
-// x86-32 has atomics up to 8 bytes
-// FIXME: Check that we actually have cmpxchg8b before setting
-// MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
-MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+setAtomic();
   }

As far as I can see, in the constructor this call is _always_ made with `CPU` 
set to `CK_Generic`, i.e. zero.  Therefore, the "allow locked atomics up to 4 
bytes" path in `setAtomic` is always chosen.  Maybe it is clearer to just 
initialize `MaxAtomicPromoteWidth` to 32 directly here, instead?



Repository:
  rL LLVM

https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

> Could someone help me figure out what is the cause and correct solution to 
> that failure? @jlebar?

The test is checking that the macros have the same value when compiling for 
CUDA host and device.  That is, if we're compiling for an x86 CPU and an NVPTX 
GPU, we invoke cc1 twice, and the macros should have the same values both 
times.  Which, I know, is a lie.  But because when we're compiling for NVPTX we 
still parse all of the CPU code, macros generally need to have the same values 
otherwise we get into Big Trouble.  NVPTX atomics are controlled separately.

You can see in NVPTXTargetInfo that we read properties from the host targetinfo 
so that we export the same macros.  The problem here seems to be that we're 
mutating the x86 targetinfo after the nvptx targetinfo reads its properties.

Does that give you enough context to fix the problem?


Repository:
  rL LLVM

https://reviews.llvm.org/D29542



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny created this revision.
Herald added a subscriber: emaste.

Set the maximum width of atomic operations on x86-32 based on the target
CPU. The 64-bit inline atomics require cmpxchg8b which is an i586
instruction. Other inline atomics require cmpxchg which is an i486
instruction.

This fixes the incorrect value of __GCC_ATOMIC_LLONG_LOCK_FREE
and __atomic_always_lock_free() on FreeBSD where clang defaults to i486
CPU (PR#31864).

TODO: one CUDA test is broken:

  /home/mgorny/llvm/_build/./bin/clang  --cuda-host-only -nocudainc -target 
x86_64-windows-msvc -x cuda -E -dM -o - /dev/null| grep 'define __[^ 
]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC'| grep -v 
'__LDBL\|_LONG_DOUBLE' > 
/home/mgorny/llvm/_build/tools/clang/test/Preprocessor/Output/x86_64-msvc-host-defines-filtered
  /home/mgorny/llvm/_build/./bin/clang  --cuda-device-only -nocudainc 
-nocudalib -target x86_64-windows-msvc -x cuda -E -dM -o - /dev/null| grep 
'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC'| grep -v 
'__LDBL\|_LONG_DOUBLE' > 
/home/mgorny/llvm/_build/tools/clang/test/Preprocessor/Output/x86_64-msvc-device-defines-filtered
  diff 
/home/mgorny/llvm/_build/tools/clang/test/Preprocessor/Output/x86_64-msvc-host-defines-filtered
 
/home/mgorny/llvm/_build/tools/clang/test/Preprocessor/Output/x86_64-msvc-device-defines-filtered
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  9,17c9,17
  < #define __GCC_ATOMIC_BOOL_LOCK_FREE 2
  < #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
  < #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
  < #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
  < #define __GCC_ATOMIC_INT_LOCK_FREE 2
  < #define __GCC_ATOMIC_LLONG_LOCK_FREE 2
  < #define __GCC_ATOMIC_LONG_LOCK_FREE 2
  < #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
  < #define __GCC_ATOMIC_SHORT_LOCK_FREE 2
  ---
  > #define __GCC_ATOMIC_BOOL_LOCK_FREE 1
  > #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 1
  > #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 1
  > #define __GCC_ATOMIC_CHAR_LOCK_FREE 1
  > #define __GCC_ATOMIC_INT_LOCK_FREE 1
  > #define __GCC_ATOMIC_LLONG_LOCK_FREE 1
  > #define __GCC_ATOMIC_LONG_LOCK_FREE 1
  > #define __GCC_ATOMIC_POINTER_LOCK_FREE 1
  > #define __GCC_ATOMIC_SHORT_LOCK_FREE 1
  19c19
  < #define __GCC_ATOMIC_WCHAR_T_LOCK_FREE 2
  ---
  > #define __GCC_ATOMIC_WCHAR_T_LOCK_FREE 1
  
  --

Could someone help me figure out what is the cause and correct solution to that 
failure? @jlebar?


Repository:
  rL LLVM

https://reviews.llvm.org/D29542

Files:
  lib/Basic/Targets.cpp
  test/CodeGen/atomic-ops.c
  test/CodeGen/ms-volatile.c
  test/CodeGenCXX/atomicinit.cpp
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -target-cpu i686 -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i486-linux-gnu -target-cpu i486 -std=c11
 
 // Basic parsing/Sema tests for __c11_atomic_*
 
@@ -14,22 +15,34 @@
 _Static_assert(__GCC_ATOMIC_SHORT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_INT_LOCK_FREE == 2, "");
 _Static_assert(__GCC_ATOMIC_LONG_LOCK_FREE == 2, "");
+#if defined(__i486__)
+_Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 1, "");
+#else
 _Static_assert(__GCC_ATOMIC_LLONG_LOCK_FREE == 2, "");
+#endif
 _Static_assert(__GCC_ATOMIC_POINTER_LOCK_FREE == 2, "");
 
 _Static_assert(__c11_atomic_is_lock_free(1), "");
 _Static_assert(__c11_atomic_is_lock_free(2), "");
 _Static_assert(__c11_atomic_is_lock_free(3), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(4), "");
+#if defined(__i486__)
+_Static_assert(__c11_atomic_is_lock_free(8), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__c11_atomic_is_lock_free(8), "");
+#endif
 _Static_assert(__c11_atomic_is_lock_free(16), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(17), ""); // expected-error {{not an integral constant expression}}
 
 _Static_assert(__atomic_is_lock_free(1, 0), "");
 _Static_assert(__atomic_is_lock_free(2, 0), "");
 _Static_assert(__atomic_is_lock_free(3, 0), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__atomic_is_lock_free(4, 0), "");
+#if defined(__i486__)
+_Static_assert(__atomic_is_lock_free(8, 0), ""); // expected-error {{not an integral constant expression}}
+#else
 _Static_assert(__atomic_is_lock_free(8, 0), "");
+#endif
 _Static_assert(__atomic_is_lock_free(16, 0), ""); // expected-error {{not an integral constant expression}}
 _Static_assert(__atomic_is_lock_free(17, 0), ""); // expected-error {{not an integral constant expression}}
 
@@ -56,13 +69,21 @@
 _Static_assert(__atomic_is_lock_free(4, ), "");