RFR: 8319777: Zero: Support 8-byte cmpxchg [v2]

[Aleksey Shipilev]

On Mon, 13 Nov 2023 21:32:33 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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.

All right, I guess that would explain what `NATIVE` means. Although for Zero case, the "native ISA" is basically compiler built-ins, which are arguably "natively" supported :) Anyway, if that makes your follow-ups easier, there is no need to spend more time dwelling on this. I pushed the update now.

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

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