RFR: 8319777: Zero: Support 8-byte cmpxchg

[David Holmes]

On Mon, 13 Nov 2023 14:25:28 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> src/hotspot/cpu/zero/globalDefinitions_zero.hpp line 30:
>> 
>>> 28: 
>>> 29: // Unconditionally supports 8-byte cmpxchg either with
>>> 30: // compiler intrinsics or with library/kernel helpers.
>> 
>> That's not what "native support for cx8" was meant to mean though - e.g. see the ARM header - it only sets this when building for ARMv7.
>> 
>> You can just leave this file alone and simply set `_supports_cx8` below to achieve the same goal. And that will fit in cleaner with the changes I am making.
>
> Well, yes, we can just do `_supports_cx8 = true`.
> 
> But I am confused by the meaning of `SUPPORTS_NATIVE_CX8`. What is it? I read it as "we know statically, at compile time, that the target platform supports CX8". Otherwise, we poll it at runtime and let the runtime code decide by checking `VMVersion::supports_cx8()`. Defining `SUPPORTS_NATIVE_CX8` compiles out access backend locking paths completely, for example, without resorting to runtime checks.
> 
> What I am missing? Is the wording for the comment misleading?

Yeah it is something that can be read two ways and the current code is confused about it. I take it to mean there is actual native ISA support, versus there is some way of achieving the same effect. That is the way the ARM code uses it: if you build for ARMv7 then` SUPPORTS_NATIVE_CX8` is defined, otherwise runtime checks exist for `ldrex` or `kuser_helper` support.

Other platforms confuse things somewhat. Here's the definition of `supports_cx8()`:

  static bool supports_cx8()  {
#ifdef SUPPORTS_NATIVE_CX8
    return true;
#else
    return _supports_cx8;
#endif
  }

So if you define `SUPPORTS_NATIVE_CX8` then `supports_cx8()` is always true - no runtime checks involved, no read of `_supports_cx8`. You've indicated that you have built a binary to only run where there is native CX8 support. Otherwise you should use runtime checks to set `_supports_cx8` as appropriate to control what `supports_cx8()` returns. Setting both is redundant/pointless and existing code is very confused about this. Take a look at x86 for example, it both defines SUPPORTS_NATIVE_CX8 and has ` _supports_cx8 = supports_cmpxchg8();` - but the latter is dead code as nothing will ever read it.

So for me Zero was correct to only set `SUPPORTS_NATIVE_CX8` for 64-bit, but what it failed to do was set `_supports_cx8` on 32-bit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16614#discussion_r1391728740