RFR: 8293117: Add atomic bitset functions

[Aleksey Shipilev]

On Fri, 28 Apr 2023 07:10:40 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this change that adds and/or/xor bitops to Atomic.
> 
> Some offline discussion reached consensus on the following API (with T being a
> template parameter that must be an integral type):
>   T fetch_then_and(volatile T* dest, T bits, atomic_memory_order order)
>   T fetch_then_or(volatile T* dest, T bits, atomic_memory_order order)
>   T fetch_then_xor(volatile T* dest, T bits, atomic_memory_order order)
>   T and_then_fetch(volatile T* dest, T bits, atomic_memory_order order)
>   T or_then_fetch(volatile T* dest, T bits, atomic_memory_order order)
>   T xor_then_fetch(volatile T* dest, T bits, atomic_memory_order order)
> with order defaulting to memory_order_conservative.
> 
> I'm hoping there won't be more bike-shedding on the names.
> 
> This naming convention differs from what exists for add/sub, where we have
> fetch_and_add, add_and_fetch, and the like.  fetch_and_and and and_and_fetch
> just looked too weird.  For consistency we should probably rename the add/sub
> operations to use "then".
> 
> A default implementation is provided, using a CAS loop for all operations.
> That implementation can be overridden on a per-platform basis.  Currently
> there aren't any platform-specific implementations; such will be added by
> followup RFEs.  We'll want to override for most (if not all) platforms, since
> the CAS loop implementation is generally less than optimal (in some cases
> rather comically so).
> 
> For some platforms a CAS loop is necessary to obtain either the old or the new
> value, but have a better implementation available if neither value is needed.
> However, we can't easily make use of such.  We would need to either add
> functions returning void to the API or change the API to return expression
> templates so we can detect how the result is used.  However, we may be able to
> use compiler intrinsics for some platforms, which may be able to specialize
> for the usage context.  If that turns out to be a problem then we can expand
> the API accordingly later.
> 
> Testing:
> mach5 tier1, including new gtests for the new functionality.

This look okay, a couple of comments.

src/hotspot/share/runtime/atomic.hpp line 703:

> 701:       old_value = fetched_value;
> 702:       new_value = operation(old_value);
> 703:       fetched_value = Atomic::cmpxchg(dest, old_value, new_value, order);

At some point in future, it would be nice to replace the retry-loops like these with _weak_ CASes. It could be done in platform-specific code, though, but it would seem awkward to specialize for all LL/SC-retry-loop platforms just to get access to weak CAS.

src/hotspot/share/runtime/atomic.hpp line 775:

> 773:     return bits ^ Atomic::fetch_then_xor(dest, bits, order);
> 774:   }
> 775: };

Is this used somewhere? Looks like all current helpers use CASes instead of this "prefetch" code.

test/hotspot/gtest/runtime/test_atomic.cpp line 287:

> 285: TEST(AtomicBitopsTest, int64) {
> 286:   AtomicBitopsTestSupport<int64_t>()();
> 287: }

These 64-bit tests would fail on 32-bit systems, are they not? They are outside the requirements for the contract to only support `sizeof(int)` and `sizeof(void*`). Probably just `#ifdef _LP64` these?

-------------

PR Review: https://git.openjdk.org/jdk/pull/13711#pullrequestreview-1405464212
PR Review Comment: https://git.openjdk.org/jdk/pull/13711#discussion_r1180086805
PR Review Comment: https://git.openjdk.org/jdk/pull/13711#discussion_r1180097923
PR Review Comment: https://git.openjdk.org/jdk/pull/13711#discussion_r1180099057