Handle constant "pointers" for `__atomic_always_lock_free`/`__atomic_is_lock_free`. #99340

jyknight · 2024-07-17T15:24:08Z

The second argument passed to these builtins is used to validate whether the object's alignment is sufficient for atomic operations of the given size.

Currently, the builtins can be folded at compile time only when the argument is 0/nullptr, or if the type of the pointer guarantees appropriate alignment.

This change allows the compiler to also evaluate non-null constant pointers, which enables callers to check a specified alignment, instead of only the type or an exact object. E.g.:
__atomic_is_lock_free(sizeof(T), (void*)4)
can be potentially evaluated to true at compile time, instead of generating a libcall. This is also supported by GCC, and used by libstdc++, and is also useful for libc++'s atomic_ref.

Also helps with (but doesn't fix) issue #75081.

This also fixes a crash bug, when the second argument was a non-pointer implicitly convertible to a pointer (such as an array, or a function).

…is_lock_free`. The second argument passed to these builtins is used to validate whether the object's alignment is sufficient for atomic operations of the given size. Currently, the builtins can be folded at compile time only when the argument is 0/nullptr, or if the _type_ of the pointer guarantees appropriate alignment. This change allows the compiler to also evaluate non-null constant pointers, which enables callers to check a specified alignment, instead of only the type or an exact object. E.g.: `__atomic_is_lock_free(sizeof(T), (void*)4)` can be potentially evaluated to true at compile time, instead of generating a libcall. This is also supported by GCC, and used by libstdc++. Related to (but doesn't fix) issue llvm#75081.

llvmbot · 2024-07-17T15:24:38Z

@llvm/pr-subscribers-clang

Author: James Y Knight (jyknight)

Changes

The second argument passed to these builtins is used to validate whether the object's alignment is sufficient for atomic operations of the given size.

Currently, the builtins can be folded at compile time only when the argument is 0/nullptr, or if the type of the pointer guarantees appropriate alignment.

This change allows the compiler to also evaluate non-null constant pointers, which enables callers to check a specified alignment, instead of only the type or an exact object. E.g.:
__atomic_is_lock_free(sizeof(T), (void*)4)
can be potentially evaluated to true at compile time, instead of generating a libcall. This is also supported by GCC, and used by libstdc++, and is also useful for libc++'s atomic_ref.

Also helps with (but doesn't fix) issue #75081.

Full diff: https://github.com/llvm/llvm-project/pull/99340.diff

2 Files Affected:

(modified) clang/lib/AST/ExprConstant.cpp (+13-5)
(modified) clang/test/Sema/atomic-ops.c (+18)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 0aeac9d03eed3..6eb6d27cc9588 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -12949,13 +12949,21 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
           Info.Ctx.getTargetInfo().getMaxAtomicInlineWidth();
       if (Size <= Info.Ctx.toCharUnitsFromBits(InlineWidthBits)) {
         if (BuiltinOp == Builtin::BI__c11_atomic_is_lock_free ||
-            Size == CharUnits::One() ||
-            E->getArg(1)->isNullPointerConstant(Info.Ctx,
-                                                Expr::NPC_NeverValueDependent))
-          // OK, we will inline appropriately-aligned operations of this size,
-          // and _Atomic(T) is appropriately-aligned.
+            Size == CharUnits::One())
           return Success(1, E);
 
+        // If the pointer argument can be evaluated to a compile-time constant
+        // integer (or nullptr), check if that value is appropriately aligned.
+        const Expr *PtrArg = E->getArg(1);
+        Expr::EvalResult ExprResult;
+        APSInt IntResult;
+        if (PtrArg->EvaluateAsRValue(ExprResult, Info.Ctx) &&
+            ExprResult.Val.toIntegralConstant(IntResult, PtrArg->getType(),
+                                              Info.Ctx) &&
+            IntResult.isAligned(Size.getAsAlign()))
+          return Success(1, E);
+
+        // Otherwise, check if the type's alignment against Size.
         QualType PointeeType = E->getArg(1)->IgnoreImpCasts()->getType()->
           castAs<PointerType>()->getPointeeType();
         if (!PointeeType->isIncompleteType() &&
diff --git a/clang/test/Sema/atomic-ops.c b/clang/test/Sema/atomic-ops.c
index 9b82d82ff8269..86b6ec6ba7a25 100644
--- a/clang/test/Sema/atomic-ops.c
+++ b/clang/test/Sema/atomic-ops.c
@@ -124,6 +124,24 @@ _Static_assert(__atomic_always_lock_free(4, &i64), "");
 _Static_assert(!__atomic_always_lock_free(8, &i32), "");
 _Static_assert(__atomic_always_lock_free(8, &i64), "");
 
+// Validate use with fake pointers constants. This mechanism is used to allow
+// validating atomicity of a given size and alignment.
+_Static_assert(__atomic_is_lock_free(1, (void*)1), "");
+_Static_assert(__atomic_is_lock_free(1, (void*)-1), "");
+_Static_assert(__atomic_is_lock_free(4, (void*)2), ""); // expected-error {{not an integral constant expression}}
+_Static_assert(__atomic_is_lock_free(4, (void*)-2), ""); // expected-error {{not an integral constant expression}}
+_Static_assert(__atomic_is_lock_free(4, (void*)4), "");
+_Static_assert(__atomic_is_lock_free(4, (void*)-4), "");
+
+_Static_assert(__atomic_always_lock_free(1, (void*)1), "");
+_Static_assert(__atomic_always_lock_free(1, (void*)-1), "");
+_Static_assert(!__atomic_always_lock_free(4, (void*)2), "");
+_Static_assert(!__atomic_always_lock_free(4, (void*)-2), "");
+_Static_assert(__atomic_always_lock_free(4, (void*)4), "");
+_Static_assert(__atomic_always_lock_free(4, (void*)-4), "");
+
+
+
 #define _AS1 __attribute__((address_space(1)))
 #define _AS2 __attribute__((address_space(2)))

AaronBallman

Thanks for this! It should probably have a release note + documentation so users know this is a supported behavior?

AaronBallman · 2024-07-17T16:22:17Z

clang/test/Sema/atomic-ops.c

+_Static_assert(!__atomic_always_lock_free(4, (void*)2), "");
+_Static_assert(!__atomic_always_lock_free(4, (void*)-2), "");
+_Static_assert(__atomic_always_lock_free(4, (void*)4), "");
+_Static_assert(__atomic_always_lock_free(4, (void*)-4), "");


Should we also add some test coverage where the fake pointer is a constant expression but utter nonsense? e.g.,

__atomic_always_lock_free(1, (void*)"well would you look at this") __atomic_always_lock_free(1, (void*)42.0f)

(Just to make sure that the conversion correctly ignores these as valid hints.)

Neat, found a pre-existing bug: _Static_assert(__atomic_always_lock_free(2, "string"), ""); triggers an assertion failure in clang at head because the type of "string" is ConstantArrayType not PointerType, in this cast:

llvm-project/clang/lib/AST/ExprConstant.cpp

Line 12960 in 83fbd79

castAs<PointerType>()->getPointeeType();

The argument AST looks like:

(gdb) p E->getArg(1)->dump() ImplicitCastExpr 0x1463b528 'const volatile void *' <BitCast> `-ImplicitCastExpr 0x1463b510 'char *' <ArrayToPointerDecay> `-StringLiteral 0x1463b440 'char[7]' lvalue "string"

I suppose it shouldn't do "IgnoreImpCasts", and instead only ignore a single level of BitCast ImplicitCastExpr?

I think it should undergo an actual lvalue to rvalue conversion on the operand so that we get all the expected conversions like decay. We have the same issue with: https://godbolt.org/z/P69vx5W8a or with the even-more-cursed: https://godbolt.org/z/zxTbbWbdc

It does undergo such conversions already -- it's just that we were stripping them all off with the IgnoreImpCasts call. I've pushed what I think is a valid fix, PTAL.

Ah, you're right, sorry for that!

github-actions · 2024-07-19T17:19:14Z

✅ With the latest revision this PR passed the C/C++ code formatter.

AaronBallman

LGTM but please add a release note when landing so users know about the changes.

…is_lock_free`. (llvm#99340) The second argument passed to these builtins is used to validate whether the object's alignment is sufficient for atomic operations of the given size. Currently, the builtins can be folded at compile time only when the argument is 0/nullptr, or if the _type_ of the pointer guarantees appropriate alignment. This change allows the compiler to also evaluate non-null constant pointers, which enables callers to check a specified alignment, instead of only the type or an exact object. E.g.: `__atomic_is_lock_free(sizeof(T), (void*)4)` can be potentially evaluated to true at compile time, instead of generating a libcall. This is also supported by GCC, and used by libstdc++, and is also useful for libc++'s atomic_ref. Also helps with (but doesn't fix) issue llvm#75081. This also fixes a crash bug, when the second argument was a non-pointer implicitly convertible to a pointer (such as an array, or a function).

…is_lock_free`. (#99340) The second argument passed to these builtins is used to validate whether the object's alignment is sufficient for atomic operations of the given size. Currently, the builtins can be folded at compile time only when the argument is 0/nullptr, or if the _type_ of the pointer guarantees appropriate alignment. This change allows the compiler to also evaluate non-null constant pointers, which enables callers to check a specified alignment, instead of only the type or an exact object. E.g.: `__atomic_is_lock_free(sizeof(T), (void*)4)` can be potentially evaluated to true at compile time, instead of generating a libcall. This is also supported by GCC, and used by libstdc++, and is also useful for libc++'s atomic_ref. Also helps with (but doesn't fix) issue #75081. This also fixes a crash bug, when the second argument was a non-pointer implicitly convertible to a pointer (such as an array, or a function).

jyknight requested review from AaronBallman and efriedma-quic July 17, 2024 15:24

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 17, 2024

AaronBallman reviewed Jul 17, 2024

View reviewed changes

jyknight mentioned this pull request Jul 19, 2024

[libc++] Fix bug in atomic_ref's calculation of lock_free-ness #99570

Merged

Fix bug with implicit casts from non-pointer types.

3e6440c

fix clang-format issue.

69e4fa4

AaronBallman approved these changes Jul 22, 2024

View reviewed changes

jyknight added 2 commits July 22, 2024 13:59

Merge remote-tracking branch 'origin/main' into atomic-lock-free-intptr

9ea6910

Add release note

4e3c491

jyknight merged commit 511e93b into llvm:main Jul 22, 2024
5 of 6 checks passed

jyknight deleted the atomic-lock-free-intptr branch July 22, 2024 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle constant "pointers" for `__atomic_always_lock_free`/`__atomic_is_lock_free`. #99340

Handle constant "pointers" for `__atomic_always_lock_free`/`__atomic_is_lock_free`. #99340

jyknight commented Jul 17, 2024 •

edited

Loading

llvmbot commented Jul 17, 2024

AaronBallman left a comment

AaronBallman Jul 17, 2024

jyknight Jul 17, 2024

AaronBallman Jul 18, 2024

jyknight Jul 19, 2024

AaronBallman Jul 22, 2024

github-actions bot commented Jul 19, 2024 •

edited

Loading

AaronBallman left a comment

Handle constant "pointers" for __atomic_always_lock_free/__atomic_is_lock_free. #99340

Handle constant "pointers" for __atomic_always_lock_free/__atomic_is_lock_free. #99340

Conversation

jyknight commented Jul 17, 2024 • edited Loading

llvmbot commented Jul 17, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Jul 17, 2024

Choose a reason for hiding this comment

jyknight Jul 17, 2024

Choose a reason for hiding this comment

AaronBallman Jul 18, 2024

Choose a reason for hiding this comment

jyknight Jul 19, 2024

Choose a reason for hiding this comment

AaronBallman Jul 22, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 19, 2024 • edited Loading

AaronBallman left a comment

Choose a reason for hiding this comment

Handle constant "pointers" for `__atomic_always_lock_free`/`__atomic_is_lock_free`. #99340

Handle constant "pointers" for `__atomic_always_lock_free`/`__atomic_is_lock_free`. #99340

jyknight commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading