Page MenuHomePhabricator

[compiler-rt] Fix atomic support functions on 32-bit architectures
ClosedPublic

Authored by arichardson on Aug 25 2020, 12:47 AM.

Details

Summary

The code currently uses c11_atomic_is_lock_free() to detect whether an
atomic operation is natively supported. However, this can result in a
runtime function call to determine whether the given operation is lock-free
and clang generating a call to e.g.
atomic_load_8 since the branch is
not a constant zero. Since we are implementing those runtime functions, we
must avoid those calls. This patch replaces c11_atomic_is_lock_free()
with
atomic_always_lock_free() which always results in a compile-time
constant value. This problem was found while compiling atomic.c for MIPS32
since the -Watomic-alignment warning was being triggered and objdump showed
an undefined reference to _atomic_is_lock_free.

In addition to fixing 32-bit platforms this also enables the 16-byte case
that was disabled in r153779 (185f2edd70a34d28b305df0cd8ce519ecbca2cfd).

Diff Detail

Event Timeline

arichardson created this revision.Aug 25 2020, 12:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: Restricted Project, jfb, atanasyan and 2 others. · View Herald Transcript
arichardson requested review of this revision.Aug 25 2020, 12:47 AM

remove incorrect cast

luismarques accepted this revision.Aug 25 2020, 1:37 AM

LGTM. (I had already gone down roughly the same path in D86281. This patch additionally adds checks for the alignments of the pointers, so it can supersede D86281)

This revision is now accepted and ready to land.Aug 25 2020, 1:37 AM

This problem was found while compiling atomic.c for MIPS32 since the -Watomic-alignment warning was being triggered and objdump showed an undefined reference to _atomic_is_lock_free.

Are you sure this fixes the issue properly? If we're triggering -Watomic-alignment, probably the types are wrong somewhere. (In particular, some targets support lock-free operations on _Atomic long long, but the ABI alignment of long long is lower than 8.)


Have you seen https://reviews.llvm.org/D45321 ?

This problem was found while compiling atomic.c for MIPS32 since the -Watomic-alignment warning was being triggered and objdump showed an undefined reference to _atomic_is_lock_free.

Are you sure this fixes the issue properly? If we're triggering -Watomic-alignment, probably the types are wrong somewhere. (In particular, some targets support lock-free operations on _Atomic long long, but the ABI alignment of long long is lower than 8.)

We are triggering -Watomic-alignment because __c11_atomic_is_lock_free(8) expands to a __atomic_is_lock_free call for mips32 since the property is not known statically, so the branch inside the switch emitted and contains a call to __atomic_load_8 which triggers the warning. With this patch we know that 8-byte atomics are not always lock free so the branch is not emitted and we fall back to the locking code.


Have you seen https://reviews.llvm.org/D45321 ?

Thanks, I had not seen that review and it seems like it does almost the same thing, but uses the lock-free code in more cases.
It seems like both clang and GCC assume that atomic operations on pointers with unknown alignment are not always lock free: https://godbolt.org/z/T3oj1r
Therefore, this approach should be safe? And the optimization that checks the alignment and uses the atomic instructions instead of locks in the aligned case could be added later?

We are triggering -Watomic-alignment because __c11_atomic_is_lock_free(8) expands to a __atomic_is_lock_free call for mips32 since the property is not known statically, so the branch inside the switch emitted and contains a call to __atomic_load_8 which triggers the warning.

Which -Watomic-alignment warning are we talking about? The "max lock-free size" one, or the "actual alignment" one?

Therefore, this approach should be safe? And the optimization that checks the alignment and uses the atomic instructions instead of locks in the aligned case could be added later?

The way the code with this patch checks for "lock-free" assumes the pointer argument is aligned the same way an a pointer to a uintN_t would be. That isn't correct in general.

Therefore, this approach should be safe? And the optimization that checks the alignment and uses the atomic instructions instead of locks in the aligned case could be added later?

The way the code with this patch checks for "lock-free" assumes the pointer argument is aligned the same way an a pointer to a uintN_t would be. That isn't correct in general.

Oh, wait, is the pointer argument here actually a void*? __atomic_always_lock_free will never succeed in that case.

dim added a comment.Aug 26 2020, 2:54 PM

We are triggering -Watomic-alignment because __c11_atomic_is_lock_free(8) expands to a __atomic_is_lock_free call for mips32 since the property is not known statically, so the branch inside the switch emitted and contains a call to __atomic_load_8 which triggers the warning.

Which -Watomic-alignment warning are we talking about? The "max lock-free size" one, or the "actual alignment" one?

The warning: misaligned atomic operation may incur significant performance penalty one.

In D86510#2240363, @dim wrote:

We are triggering -Watomic-alignment because __c11_atomic_is_lock_free(8) expands to a __atomic_is_lock_free call for mips32 since the property is not known statically, so the branch inside the switch emitted and contains a call to __atomic_load_8 which triggers the warning.

Which -Watomic-alignment warning are we talking about? The "max lock-free size" one, or the "actual alignment" one?

The warning: misaligned atomic operation may incur significant performance penalty one.

I can't seem to reproduce that... but it would indicate a bug in the code unrelated to the call to __atomic_is_lock_free. atomic.c shouldn't be trying to use misaligned lock-free atomics.

arichardson added a comment.EditedAug 26 2020, 5:12 PM
In D86510#2240363, @dim wrote:

We are triggering -Watomic-alignment because __c11_atomic_is_lock_free(8) expands to a __atomic_is_lock_free call for mips32 since the property is not known statically, so the branch inside the switch emitted and contains a call to __atomic_load_8 which triggers the warning.

Which -Watomic-alignment warning are we talking about? The "max lock-free size" one, or the "actual alignment" one?

The warning: misaligned atomic operation may incur significant performance penalty one.

I can't seem to reproduce that... but it would indicate a bug in the code unrelated to the call to __atomic_is_lock_free. atomic.c shouldn't be trying to use misaligned lock-free atomics.

See https://godbolt.org/z/h91ob3. Seems to be the large one not the alignment one for MIPS32.

luismarques resigned from this revision.Aug 27 2020, 6:31 AM

I'll leave this to the experts then! :-)

This revision now requires review to proceed.Aug 27 2020, 6:31 AM

Use the lock-free code when

a) atomic ops of that size are always lock-free
b) aligned atomics ops are lock-free and the pointer is aligned

Note: the optimized functions assume that the pointer is aligned since the argument is a uint64_t, etc. If this is not correct the signature should be changed to use void* instead.

Note: the optimized functions assume that the pointer is aligned since the argument is a uint64_t

This should be fine.

compiler-rt/lib/builtins/atomic.c
126

Please use 0, not NULL.

p & size probably doesn't do what you want; do you mean p % size?

Use 0 instead of NULL and fix broken alignment check

This revision is now accepted and ready to land.Wed, Sep 16, 7:32 PM