[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[James Y Knight via Phabricator via cfe-commits]

jyknight requested changes to this revision.
jyknight added a comment.
This revision now requires changes to proceed.

This isn't correct.

The atomic interface is designed to be forward-compatible with new CPUs that 
have wider atomic instructions. That clang has a MaxAtomicPromoteWidth value 
_at all_ is unfortunate, because it breaks this ability. We've locked ourselves 
into an unfortunate ABI here -- and what's worse, it's incompatible with GCC's 
ABI.

But, the MaxAtomicPromoteWidth value impactsaffects C11 _Atomic (and therefore 
libc++ std::atomic) variables' default alignment. But, the user can in any case 
specify additional alignment. For the __atomic_is_lock_free function, 
MaxAtomicPromoteWidth is irrelevant -- it can be used on variables of any size 
or alignment and needs to return correct answers, even on future CPUs.

It's not good that clang doesn't provide the ability to create a working 
libatomic, but the right answer is to fix that, not add hacks like this. I've 
started writing up a proposal on what we need to do there, but need to finish 
that up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72579



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

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

From https://gcc.gnu.org/ml/gcc/2020-02/msg00116.html

> We discussed this on IRC in #gcc. There was no motivation to change GCC. The 
> platform that wants to avoid the libatomic dependency doesn't use GCC anyway. 
> Relying on not getting the libatomic dependency seems fragile (it only works 
> for a specific codebase, and if some other is_lock_free check is added to the 
> codebase, the libatomic dependency will return anyway).

So they will not extend the interface. Clang has `getMaxAtomicPromoteWidth()`. 
Do we need the extension? I'm happy with either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72579



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

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Eagerly evaluating based on MaxAtomicPromoteWidth seems fine... assuming we're 
actually setting MaxAtomicPromoteWidth to something appropriate.  The value on 
PowerPC looks wrong.

If we're worried about constant-evaluating it in contexts where gcc doesn't, we 
can generate a diagnostic to prevent that from happening.  (See the handling of 
Builtin::BIstrcmp.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72579



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

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[JF Bastien via Phabricator via cfe-commits]

jfb added a subscriber: jwakely.
jfb added a comment.

This changes the expression to a constant expression as well, right? You should 
point this out in the commit message.

The divergence with GCC is unfortunate, @jwakely do you think you'd be able to 
get GCC to match this behavior as well? It's technically a breaking change, but 
I doubt it matters in practice. It might also break code when newer 
architectures come in (if they have larger atomics) but again I think that's 
fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72579



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

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

If I understand this correctly, this just evaluates the query for lock free 
atomics at compile time if the size is larger than the maximum possible size 
for an atomic on the target. If that's the case, this looks fine to me. But of 
course, some of the other target maintainers might feel otherwise.
The incompatibility with GCC might be an issue for some, but I don't expect 
this to be an issue at least on PPC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72579



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

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61771 tests passed, 0 failed 
and 780 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72579



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

[PATCH] D72579: Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: adalava, dim, nemanjai, jfb, rsmith.
Herald added subscribers: cfe-commits, steven.zhang, dexonsmith, krytarowski, 
arichardson, emaste.
Herald added a project: clang.

MaxAtomicPromoteWidth is defined as "the maximum width lock-free atomic
operation which will ever be supported for the given target", so an
oversized __c11_atomic_is_lock_free() or __atomic_is_lock_free() can be
evaluated to 0. This is advantageous in some scenarios (e.g. FreeBSD
powerpc, see D71600 ) to avoid the dependency 
on libatomic.

The behavior of __atomic_is_lock_free() will diverge from GCC as GCC
never evaluates it to 0 (`gcc/builtins.c:fold_builtin_atomic_always_lock_free`).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72579

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/CodeGen/atomic-ops.c
  clang/test/CodeGen/big-atomic-ops.c
  clang/test/Sema/atomic-ops.c


Index: clang/test/Sema/atomic-ops.c
===
--- clang/test/Sema/atomic-ops.c
+++ clang/test/Sema/atomic-ops.c
@@ -36,23 +36,23 @@
 _Static_assert(__c11_atomic_is_lock_free(3), ""); // expected-error {{not an 
integral constant expression}}
 _Static_assert(__c11_atomic_is_lock_free(4), "");
 _Static_assert(__c11_atomic_is_lock_free(8), "");
-_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(!__c11_atomic_is_lock_free(16), "");
+_Static_assert(!__c11_atomic_is_lock_free(17), "");
 
 _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), "");
 _Static_assert(__atomic_is_lock_free(8, 0), "");
-_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}}
+_Static_assert(!__atomic_is_lock_free(16, 0), "");
+_Static_assert(!__atomic_is_lock_free(17, 0), "");
 
 _Static_assert(atomic_is_lock_free((atomic_char*)0), "");
 _Static_assert(atomic_is_lock_free((atomic_short*)0), "");
 _Static_assert(atomic_is_lock_free((atomic_int*)0), "");
 _Static_assert(atomic_is_lock_free((atomic_long*)0), "");
 // expected-error@+1 {{__int128 is not supported on this target}}
-_Static_assert(atomic_is_lock_free((_Atomic(__int128)*)0), ""); // 
expected-error {{not an integral constant expression}}
+_Static_assert(!atomic_is_lock_free((_Atomic(__int128)*)0), "");
 _Static_assert(atomic_is_lock_free(0 + (atomic_char*)0), "");
 
 char i8;
Index: clang/test/CodeGen/big-atomic-ops.c
===
--- clang/test/CodeGen/big-atomic-ops.c
+++ clang/test/CodeGen/big-atomic-ops.c
@@ -204,9 +204,6 @@
   // CHECK: call i32 @__atomic_is_lock_free(i64 16, i8* {{.*}}@sixteen{{.*}})
   __atomic_is_lock_free(16, );
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 17, i8* {{.*}}@seventeen{{.*}})
-  __atomic_is_lock_free(17, );
-
   // CHECK: call i32 @__atomic_is_lock_free(i64 4, {{.*}})
   __atomic_is_lock_free(4, incomplete);
 
@@ -215,6 +212,8 @@
   __atomic_is_lock_free(4, cs+1);
 
   // CHECK-NOT: call
+  __atomic_is_lock_free(17, );
+  __atomic_is_lock_free(32, 0);
   __atomic_always_lock_free(3, 0);
   __atomic_always_lock_free(16, 0);
   __atomic_always_lock_free(17, 0);
Index: clang/test/CodeGen/atomic-ops.c
===
--- clang/test/CodeGen/atomic-ops.c
+++ clang/test/CodeGen/atomic-ops.c
@@ -346,11 +346,8 @@
   // CHECK: call i32 @__atomic_is_lock_free(i32 3, i8* null)
   __c11_atomic_is_lock_free(3);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 16, i8* {{.*}}@sixteen{{.*}})
-  __atomic_is_lock_free(16, );
-
-  // CHECK: call i32 @__atomic_is_lock_free(i32 17, i8* {{.*}}@seventeen{{.*}})
-  __atomic_is_lock_free(17, );
+  // CHECK: call i32 @__atomic_is_lock_free(i32 8, i8* {{.*}}@seventeen{{.*}})
+  __atomic_is_lock_free(8, );
 
   // CHECK: call i32 @__atomic_is_lock_free(i32 4, {{.*}})
   __atomic_is_lock_free(4, incomplete);
@@ -360,6 +357,8 @@
   __atomic_is_lock_free(4, cs+1);
 
   // CHECK-NOT: call
+  __atomic_is_lock_free(9, );
+  __atomic_is_lock_free(16, 0);
   __atomic_always_lock_free(3, 0);
   __atomic_always_lock_free(16, 0);
   __atomic_always_lock_free(17, 0);
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11152,6 +11152,9 @@
 
 // Check power-of-two.
 CharUnits Size = CharUnits::fromQuantity(SizeVal.getZExtValue());
+