RFR: 8316961: Fallback implementations for 64-bit Atomic::{add,xchg} on 32-bit platforms [v2]
Aleksey Shipilev
shade at openjdk.org
Fri Oct 20 12:03:58 UTC 2023
On Fri, 20 Oct 2023 10:54:42 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:
>> See the bug for rationale. Looks like there is enough infrastructure to achieve what we want without significant fan-out. I checked all `atomic_*.hpp` headers for unimplemented `PlatformAdd<8>` and `PlatformXchg<8>`, and only these seem to be affected.
>>
>> Unfortunately, we cannot test these apart from the existing gtest.
>>
>> Additional testing:
>> - [x] linux-x86-server-fastdebug, atomic tests pass
>> - [x] linux-arm-server-fastdebug, atomic tests pass (with #16269 applied)
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
>
> - Merge branch 'master' into JDK-8316961-fallback-atomics
> - Work
All right, hear me out. I am trying to briefly recap where we are and where we are going from here.
>From the perspective of caller, the `Atomic` contract is ambiguous. On one hand, it says to check for `supports_cx8()`:
// Atomic operations on int64 types are not available on all 32-bit
// platforms. If atomic ops on int64 are defined here they must only
// be used from code that verifies they are available at runtime and
// can provide an alternative action if not - see supports_cx8() for
// a means to test availability.
...and on the other hand, it requires platforms to implement 8-byte CAS:
// Platform-specific implementation of cmpxchg. Support for sizes
// of 1, 4, and 8 are required.
...
template<size_t byte_size> struct PlatformCmpxchg;
This PR implicitly relies on second assumption. I think the only platform left that needs the `supports_cx8()` is ARM <= 6. The fact that current ARM code does not do consistent support for 64-bit atomics for all platforms is regrettable. To recap, while ARM 7 works fine, the situation with ARM <=6 is more complicated:
Before this PR:
- `Atomic` unit test would pass
- Attempt to use 8-byte cmpxchg would fail at runtime (either assert or `stop` in stub)
- Attempt to use 8-byte add or xchg would fail to link
After this PR:
- `Atomic` unit test would pass (now that it checks for `supports_cx8()`)
- Attempt to use 8-byte cmpxchg would fail at runtime (either assert or `stop` in stub)
- Attempt to use 8-byte add or xchg would fail at runtime (either assert or `stop` in stub)
So, I don't think this qualifies as regression for ARM <= 6: we are trading the link-time failure for all ARM platforms for clean runtime failure on ARM <= 6 platform, *if* we violate the `supports_cx8()` contract. What we gain with this PR, is that we are able to do 64-bit atomics when `supports_cx8()` is actually true on x86_32 and ARM v7, even if we don't check it.
I dabbled a little in trying to implement 8-byte CAS (locked) implementation for ARM <= 6, and I think it would require some rewrites. There are cases, for example, when we are doing creepy non-atomic stuff when `!os:is_MP` if no hardware support is available. If we go locking route, I think we would also need to check if the atomic load/stores need to be covered by the same lock.
Once/if we fix ARM <= 6, that would eliminate the need for `supports_cx8()` altogether. And I think that is a good thing, because frankly it should not be an `Atomic` caller responsibility to handle obscure platforms in 2023.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/16252#issuecomment-1772605369
More information about the hotspot-dev
mailing list