Page MenuHomePhabricator

Evaluate __{c11_,}atomic_is_lock_free to 0 (avoid libcall) if larger than MaxAtomicPromoteWidth
AbandonedPublic

Authored by MaskRay on Jan 12 2020, 5:27 PM.

Details

Summary

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).

Diff Detail

Event Timeline

MaskRay created this revision.Jan 12 2020, 5:27 PM

Unit tests: pass. 61771 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

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

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.

jfb added a subscriber: jwakely.Jan 13 2020, 10:09 AM

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.

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.)

MaskRay edited the summary of this revision. (Show Details)Jan 22 2020, 11:31 AM

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.

jyknight requested changes to this revision.Feb 21 2020, 10:39 AM
jyknight added a subscriber: jyknight.

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.

This revision now requires changes to proceed.Feb 21 2020, 10:39 AM
MaskRay abandoned this revision.Feb 21 2020, 11:18 AM