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

Jatin Bhateja jbhateja at openjdk.org
Tue May 6 08:49:58 UTC 2025


On Tue, 6 May 2025 00:30:23 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

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

DONE

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

Yes. I was facing compilation issues with raw array types.

> 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

Moved VM_Features to VM_Version.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2075014479
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2075012045
PR Review Comment: https://git.openjdk.org/jdk/pull/24329#discussion_r2075015126


More information about the graal-dev mailing list