Page MenuHomePhabricator

[clang][compiler-rt][atomics] Add `__c11_atomic_fetch_nand` builtin and support `__atomic_fetch_nand` libcall
ClosedPublic

Authored by lkail on Oct 24 2021, 10:21 PM.

Details

Summary

Add __c11_atomic_fetch_nand builtin to language extensions and support __atomic_fetch_nand libcall in compiler-rt.

Diff Detail

Event Timeline

lkail created this revision.Oct 24 2021, 10:21 PM
lkail requested review of this revision.Oct 24 2021, 10:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2021, 10:21 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
theraven accepted this revision.Oct 26 2021, 3:11 AM

Looks good to me, thanks!

I missed the clang patches, but it looks as if they didn't update clang's LanguageExtensions.html to document the new builtins - please can you add them?

This revision is now accepted and ready to land.Oct 26 2021, 3:11 AM
lkail added a comment.EditedOct 26 2021, 7:08 PM

I missed the clang patches, but it looks as if they didn't update clang's LanguageExtensions.html to document the new builtins - please can you add them?

Looks __atomic_fetch_nand is a GNU compatible builtin and it has been mentioned roughly since https://reviews.llvm.org/D10847#C226804NL1718. Do we have to document these GNU compatible __atomic_* builtins in another section?

Ah, it looks as if you didn't add the __c11_ variant of the builtin? Please can you add this?

The GCC builtins paint the ABI into a corner. They accept non-_Atomic-qualified types (the C11 spec guarantees only that these operations work on _Atomic types). The goal of the original C++ specification was to allow implementations to use atomic operations for register-sized chunks and fall back to an implementation with an inline lock for larger types, so std::atomic<T> would either have a single field of T or a std::atomic_flag field and a T field, depending on the size of T. The goal of the C11 import was to allow _Atomic(T) to be ABI-compatible with std::atomic<T>. Implementing this requires that _Atomic(T) be allowed to have a different representation to T. GCC messed this up and defined builtins that took a T*, not an _Atomic(T*), which forced all GCC-compatible ABIs to have the same representation for T and _Atomic(T). This, in turn, meant that the atomics support library couldn't use inline locks, and had to maintain a pool of locks to use for different types. This, in turn, means that _Atomic(T) silently fails in surprising ways in shared memory, some of the time, depending on the target CPU. This is a horrible mess and I would like to ensure that we always provide builtins that allow target ABIs to do the right thing, even if Linux and *BSD are trapped in a broken ABI by GCC and legacy compatibility.

lkail updated this revision to Diff 382618.Oct 27 2021, 5:30 AM
lkail retitled this revision from [compiler-rt][atomics] Support __atomic_fetch_nand to [clang][compiler-rt][atomics] Add `__c11_atomic_fetch_nand` builtin and support `__atomic_fetch_nand` libcall.
lkail edited the summary of this revision. (Show Details)

Add __c11_atomic_fetch_nand in clang; Use __c11_atomic_fetch_nand to implement lock-free __atomic_fetch_nand libcall; Updated LanguageExtensions.rst.

Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 5:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lkail requested review of this revision.Oct 27 2021, 5:31 AM

The GCC builtins paint the ABI into a corner. They accept non-_Atomic-qualified types (the C11 spec guarantees only that these operations work on _Atomic types). The goal of the original C++ specification was to allow implementations to use atomic operations for register-sized chunks and fall back to an implementation with an inline lock for larger types, so std::atomic<T> would either have a single field of T or a std::atomic_flag field and a T field, depending on the size of T. The goal of the C11 import was to allow _Atomic(T) to be ABI-compatible with std::atomic<T>. Implementing this requires that _Atomic(T) be allowed to have a different representation to T. GCC messed this up and defined builtins that took a T*, not an _Atomic(T*), which forced all GCC-compatible ABIs to have the same representation for T and _Atomic(T). This, in turn, meant that the atomics support library couldn't use inline locks, and had to maintain a pool of locks to use for different types. This, in turn, means that _Atomic(T) silently fails in surprising ways in shared memory, some of the time, depending on the target CPU. This is a horrible mess and I would like to ensure that we always provide builtins that allow target ABIs to do the right thing, even if Linux and *BSD are trapped in a broken ABI by GCC and legacy compatibility.

Good to know, thanks for your detailed explanation!

theraven accepted this revision.Oct 27 2021, 5:33 AM

Thanks!

This revision is now accepted and ready to land.Oct 27 2021, 5:33 AM
lkail updated this revision to Diff 382624.Oct 27 2021, 5:37 AM
jyknight added inline comments.Oct 27 2021, 6:10 AM
compiler-rt/lib/builtins/atomic.c
339

Same as ATOMIC_RMW now, isn't it?

lkail added inline comments.Oct 27 2021, 6:38 AM
compiler-rt/lib/builtins/atomic.c
339

Not totally. The ATOMIC_RMW macro also accept binary op sign as its argument, i.e., in the form a op b. However, nand is ~(a & b).