RFR: 8352675: Support Intel AVX10 converged vector ISA feature detection [v12]

Tue May 6 01:33:20 UTC 2025

On Mon, 5 May 2025 04:06:02 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> - Intel AVX10[1] extends and enhances the capabilities of Intel AVX-512 to benefit all Intel® products and will be the vector ISA of choice moving into the future. 
>> - It supports a new ISA versioning scheme which simplifies the existing AVX512 feature enumeration scheme. Feature set supported by an AVX10 ISA version will be supported by all the versions above it.
>> - The initial, fully-featured version of Intel® AVX10 will be enumerated as Version 2 (denoted as Intel® AVX10.2). This will include the new ISA extension over the existing AVX512 instructions. 
>> - An early version of Intel® AVX10 (Version 1, or Intel® AVX10.1) that only enumerates the Intel® AVX-512 instruction set at 128, 256, and 512 bits will be enabled on the Granite Rapids Server for software pre-enabling.
>> 
>> This patch adds the necessary CPUID feature detection for AVX10 ISA version 1 and 2.  In terms of architectural state save restoration, AVX10 is isomorphic to AVX512 support up till Granite Rapids. State components affected by AVX10 extension include SSE, AVX, Opmask, ZMM_Hi256, and Hi16_ZMM registers. 
>> 
>> The patch has been regressed through tier1 and jvmci tests 
>> 
>> Please review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>> 
>> [1] https://www.intel.com/content/www/us/en/content-details/844829/intel-advanced-vector-extensions-10-2-intel-avx10-2-architecture-specification.html
>
> Jatin Bhateja has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision:
> 
>  - Updating comment
>  - Review comments resolutions

It does look much better now. Thanks!

Some comments/suggestions follow.

src/hotspot/cpu/x86/vm_version_x86.cpp line 853:

> 851: 
> 852:   if (cpu_family() > 4) { // it supports CPUID
> 853:     _features = _cpuid_info.feature_flags(); // These can be changed by VM settings

You don't need to change this code if you equip `VM_Features` with a copy constructor.

src/hotspot/cpu/x86/vm_version_x86.cpp line 1102:

> 1100:   size_t buf_iter = cpu_info_size;
> 1101:   for (uint64_t i = 0; i < features_vector_size(); i++) {
> 1102:     insert_features_names(features_vector_elem(i), buf + buf_iter, sizeof(buf) - buf_iter, _features_names, 64 * i);

`Abstract_VM_Version::insert_features_names` is used only on x86. You can move it to `vm_version_x86.cpp/.hpp` and adjust to new layout.

src/hotspot/cpu/x86/vm_version_x86.hpp line 707:

> 705:   //
> 706:   static bool supports_cpuid()        { return _features  != 0; }
> 707:   static bool supports_cmov()         { return (_features & CPU_CMOV) != 0; }

Since you touch this code anyway, I suggest to use this opportunity to automatically derive this code using  `CPU_FEATURE_FLAGS` macro. (As an example [1].)

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/vm_version_aarch64.hpp#L147

src/hotspot/cpu/x86/vm_version_x86.hpp line 753:

> 751:   // Feature identification which can be affected by VM settings
> 752:   //
> 753:   static bool supports_cpuid()        { return Abstract_VM_Version::vm_features_exist(); }

Is `VM_Features::_features_vector_size > 0` equivalent to `_features != 0`?

I believe you can simply drop `supports_cpuid()`. x86-32 bit port is gone and even there `cpuid` support was mandatory.

src/hotspot/share/runtime/abstract_vm_version.hpp line 51:

> 49: class VM_Features {
> 50:  public:
> 51:   using FeatureVector = uint64_t [MAX_FEATURE_VEC_SIZE];

Why did you decide to declare new type name for fixed size array type? I see you use `FeatureVector` in `vmStructs*` and JVMCI code. Does it make things simpler there?

src/hotspot/share/runtime/abstract_vm_version.hpp line 91:

> 89: 
> 90:   // CPU feature flags vector, can be affected by VM settings.
> 91:   static VM_Features _vm_target_features;

Unless we plan to migrate all platforms all at once, I suggest to move this code into `VM_Version` and keep the same names (`_features` and `_cpu_features`). Ideally, `_features` field can be moved to from `Abstract_VM_Version` to platform-specific `VM_Version`s across all platforms. But leaving it as is for now is also fine with me. 

There's a precedent: `VM_Version` already overrides `_features` field on s390 [1]. 

`VM_Features` class can start as x86-specific, but for advertisement purposes it makes sense to keep it in `abstract_vm_version.hpp`.

Alternatively, `Abstract_VM_Version::_features` can be converted from `uint64_t` to `VM_Features` and non-x86 platforms can be covered by providing overloads for currently used operators (it's mostly `|=`, `&=`, and `&`, plus convertions).

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/s390/vm_version_s390.hpp#L130

src/hotspot/share/runtime/abstract_vm_version.hpp line 97:

> 95: 
> 96:   static void sync_cpu_features() {
> 97:     memcpy(_cpu_target_features._features_vector, _vm_target_features._features_vector,

Any particular reason to use `memcpy`/`memset` and not a loop over `_features_vector` array?

I believe once you define default and copy constructors for `VM_Features`, `sync_cpu_features()` and `clear_cpu_features()` won't be needed anymore.

src/hotspot/share/runtime/abstract_vm_version.hpp line 183:

> 181:   static const char* printable_jdk_debug_level();
> 182: 
> 183:   static uint64_t features() {

Not used. Drop it.

src/hotspot/share/runtime/init.cpp line 68:

> 66: void codeCache_init();
> 67: void VM_Version_init();
> 68: void VM_Version_pre_init();

Redundant declaration.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/amd64/AMD64HotSpotVMConfig.java line 94:

> 92:     final long amd64CET_IBT = getConstant("VM_Version::CPU_CET_IBT", Long.class);
> 93:     final long amd64CET_SS = getConstant("VM_Version::CPU_CET_SS", Long.class);
> 94:     final long avx10_1 = getConstant("VM_Version::CPU_AVX10_1", Long.class);

Leave them as is. @mur47x111 plans to remove them [1].

[1] https://github.com/openjdk/jdk/pull/24329#issuecomment-2838223030

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

PR Review: https://git.openjdk.org/jdk/pull/24329#pullrequestreview-2815634822
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074470895
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074469800
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074484317
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074481382
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074502713
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074479165
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074496719
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074480203
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2073919224
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2074519224