[libc++] Increase atomic_ref's required alignment for small types
#99654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Damien L-G (dalg24) ChangesBackground discussion here #99570 (comment) Require types that are 1, 2, 4, 8, or 16 bytes in length to be aligned to at least their size to allow them to be used lock-free. cc @jyknight @ldionne Full diff: https://github.com/llvm/llvm-project/pull/99654.diff 1 Files Affected:
diff --git a/libcxx/include/__atomic/atomic_ref.h b/libcxx/include/__atomic/atomic_ref.h
index 156f1961151c1..49f6982a27f1f 100644
--- a/libcxx/include/__atomic/atomic_ref.h
+++ b/libcxx/include/__atomic/atomic_ref.h
@@ -95,10 +95,14 @@ struct __atomic_ref_base {
friend struct __atomic_waitable_traits<__atomic_ref_base<_Tp>>;
+ // require types that are 1, 2, 4, 8, or 16 bytes in length to be aligned to at least their size to allow them to be
+ // used lock-free
+ static constexpr bool __min_alignement = (sizeof(_Tp) & sizeof(_Tp - 1)) || (sizeof(_Tp) > 16) ? 0 : sizeof(_Tp);
+
public:
using value_type = _Tp;
- static constexpr size_t required_alignment = alignof(_Tp);
+ static constexpr size_t required_alignment = alignof(_Tp) > __min_alignement ? alignof(_Tp) : __min_alignement;
// The __atomic_always_lock_free builtin takes into account the alignment of the pointer if provided,
// so we create a fake pointer with a suitable alignment when querying it. Note that we are guaranteed
|
jyknight
reviewed
Jul 19, 2024
jyknight
reviewed
Jul 19, 2024
|
Thanks! LGTM, but wait for ldionne to approve. |
ldionne
requested changes
Jul 23, 2024
Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
ldionne
approved these changes
Jul 23, 2024
|
I'd like to better understand the failure mode here when passing a variable that isn't suitably aligned. It seems that with this patch it becomes pretty easy to violate the Just naively creating a What happens when the alignment requirement is not satisfied? Is it only that we will incorrectly report that the |
This is also how I understand it. |
On common platforms, you may get either silently-non-atomic accesses or a crash. |
|
Ugh...actually...lemme take that back -- we aren't using that alignment info, currently. So I am wrong about the current behavior, and you are right. What I said is what we should want to be the case, but it's not now. This PR is still an improvement, and makes the ABI change that's required. But the purpose of requiring alignment is to generate efficient accesses, and this doesn't actually get there, yet. We instead generate a libcall (which, if the object is in fact aligned to required_alignment, will be lock-free), but it's still overhead of a should-be unnecessary libcall. |
|
OK, to fix it, I think we just need: (moving decls around as needed) |
EricWF
reviewed
Jul 25, 2024
| public: | ||
| using value_type = _Tp; | ||
|
|
||
| static constexpr size_t required_alignment = alignof(_Tp); | ||
| static constexpr size_t required_alignment = alignof(_Tp) > __min_alignment ? alignof(_Tp) : __min_alignment; |
There was a problem hiding this comment.
I think we should calculate the alignment using something like this?
template <class T>
constexpr size_t calc_alignment() {
size_t align = alignof(T);
while (align <= alignof(max_align_t)) {
if (__atomic_always_lock_free(sizeof(T), align))
return align;
align *= 2;
}
return alignof(T);
}
There was a problem hiding this comment.
I really like this, I think this directly translates to what we are actually trying to achieve (which is to calculate the smallest alignment that allows us to be lockfree).
There was a problem hiding this comment.
That is not a valid implementation, because it causes the ABI of the type to change depending on the microarchitecture selection.
…ease_atomic_ref_required_alignment_of_small_types
ldionne
reviewed
Jul 30, 2024
|
/cherry-pick 59ca618 |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 1, 2024
…vm#99654) This patch increases the alignment requirement for std::atomic_ref such that we can guarantee lockfree operations more often. Specifically, we require types that are 1, 2, 4, 8, or 16 bytes in size to be aligned to at least their size to be used with std::atomic_ref. This is the case for most types, however a notable exception is `long long` on x86, which is 8 bytes in length but has an alignment of 4. As a result of this patch, one has to be more careful about the alignment of objects used with std::atomic_ref. Failure to provide a properly-aligned object to std::atomic_ref is a precondition violation and is technically UB. On the flipside, this allows us to provide an atomic_ref that is actually lockfree more often, which is an important QOI property. More information in the discussion at llvm#99570 (comment). Co-authored-by: Louis Dionne <ldionne.2@gmail.com> (cherry picked from commit 59ca618)
|
/pull-request #101492 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/2411 Here is the relevant piece of the build log for the reference: |
dalg24
deleted the
increase_atomic_ref_required_alignment_of_small_types
branch
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 2, 2024
…vm#99654) This patch increases the alignment requirement for std::atomic_ref such that we can guarantee lockfree operations more often. Specifically, we require types that are 1, 2, 4, 8, or 16 bytes in size to be aligned to at least their size to be used with std::atomic_ref. This is the case for most types, however a notable exception is `long long` on x86, which is 8 bytes in length but has an alignment of 4. As a result of this patch, one has to be more careful about the alignment of objects used with std::atomic_ref. Failure to provide a properly-aligned object to std::atomic_ref is a precondition violation and is technically UB. On the flipside, this allows us to provide an atomic_ref that is actually lockfree more often, which is an important QOI property. More information in the discussion at llvm#99570 (comment). Co-authored-by: Louis Dionne <ldionne.2@gmail.com> (cherry picked from commit 59ca618)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background discussion here #99570 (comment)
Require types that are 1, 2, 4, 8, or 16 bytes in length to be aligned to at least their size to allow them to be used lock-free.
cc @jyknight @ldionne