Page MenuHomePhabricator

[compiler-rt][builtins] Tweak checks for lock-free atomics
AbandonedPublic

Authored by luismarques on Aug 16 2020, 3:51 PM.

Details

Summary

The IS_LOCK_FREE_* macros in atomic.c were using the __c11_atomic_is_lock_free builtin. Those specific builtin calls would typically be lowered to compile-time constants (probably for all of the scenarios where compiler-rt was being used and providing atomic builtins). In some cases, though, the builtin can be lowered to a call to the runtime function __atomic_is_lock_free. An example is when targeting RISC-V rv32imac and it calls __c11_atomic_is_lock_free(8) by using the IS_LOCK_FREE_8 macro. Yet, atomic.c doesn't provide an implementation of __atomic_is_lock_free. In those scenarios you aren't able to link your program. To address this issue, this patch changes the macros to be implemented in terms of the __atomic_always_lock_free builtin, which is always lowered to a compile-time constant.

An alternative approach would be to implement the run-time function __atomic_is_lock_free, but right now that would only add run-time overhead. atomic.c implements the builtin runtime functions in terms of other atomic builtins, but the only relevant builtin here is __atomic_always_lock_free, so you might as well just use that builtin in the first place. If compiler-rt started to deviate from the current implementation approach and providing architecture-specific logic it's theoretically possible that a more fine-grained implementation of __atomic_is_lock_free could allow covering additional cases with lock-free implementations. If that ever comes to pass, we can revisit the implementation of the macros changed by this patch.

Diff Detail

Event Timeline

luismarques created this revision.Aug 16 2020, 3:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2020, 3:51 PM
Herald added subscribers: Restricted Project, s.egerton, PkmX and 3 others. · View Herald Transcript
luismarques requested review of this revision.Aug 16 2020, 3:51 PM
luismarques retitled this revision from [compiler-rt][builtins] Tweak tests for lock-free atomics to [compiler-rt][builtins] Tweak checks for lock-free atomics.Aug 16 2020, 4:08 PM
jfb added a comment.Aug 17 2020, 2:41 PM

Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?

You forgot to CC llvm-commits; please resubmit.

In D86043#2222622, @jfb wrote:

Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?

This can be tested by my previous (under review) D81348. With this patch you can compile and link that test from D81348 when targeting RISC-V rv32imac, while without this patch that test will fail to link. (Tested with a complementary GNU baremetal newlib toolchain, which doesn't even have libatomic to link to.)

jfb accepted this revision.Aug 18 2020, 8:30 AM
In D86043#2222622, @jfb wrote:

Can you add a test that this works, e.g. by having a program which uses these functions, but doesn't link to libatomic?

This can be tested by my previous (under review) D81348. With this patch you can compile and link that test from D81348 when targeting RISC-V rv32imac, while without this patch that test will fail to link. (Tested with a complementary GNU baremetal newlib toolchain, which doesn't even have libatomic to link to.)

Ah great, thanks!

This revision is now accepted and ready to land.Aug 18 2020, 8:31 AM
luismarques abandoned this revision.Aug 20 2020, 5:24 AM

This review is superseded by D86281, due to issues with subscribing llvm-commits.