On Wed, 22 Nov 2023 08:48:09 GMT, Aleksey Shipilev <[email protected]> wrote:
>> David Holmes has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge with master and update Zero code accordingly
>> - Merge branch 'master' into 8318776-supports_cx8
>> - Remove unnecessary includes of vm_version.hpp.
>> Fix copyright years.
>> - Remove cx8 comment as no longer relevant (the spinlock is used regardless
>> of cx8)
>> - Remove suports_cx8() checks from gtest
>> - Remove test for VMSupportsCX8
>> - 8318776: Require supports_cx8 to always be true
>
> src/hotspot/cpu/x86/vm_version_x86.cpp line 819:
>
>> 817: }
>> 818:
>> 819: _supports_cx8 = supports_cmpxchg8();
>
> I think we should leave the runtime check here (under `ifndef`, like in
> ARM?). This covers the remaining case of running on legacy x86 without CX8
> implemented: the init guarantee would then fire and prevent any other
> surprises at runtime. Sure, it would be hard to come up with such a platform
> today, but it would be safer to refuse to run there right away on the
> off-chance someone actually has it :)
@shipilev - Do you have a particular legacy x86 in mind?
> src/hotspot/share/runtime/vm_version.cpp line 33:
>
>> 31: void VM_Version_init() {
>> 32: VM_Version::initialize();
>> 33: guarantee(VM_Version::supports_cx8(), "Support for 64-bit atomic
>> operations in required in this release");
>
> Typo: "in required in". Also, no need to mention "this release" at all?
> Suggestion for message: "JVM requires platform support for 64-bit atomic
> operations"
Or the simpler change:
s/in required/is required/
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402515036
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402528045
