Re: RFR: 8265403: consolidate definition of CPU features [v4]
On Mon, 19 Apr 2021 20:01:46 GMT, Vladimir Kozlov wrote: >> Doug Simon has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - updated date in copyright >> - added blank lines after macros > > You need review from Runtime group too. Thanks for the reviews @vnkozlov and @iklam. - PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v4]
On Mon, 19 Apr 2021 19:56:45 GMT, Doug Simon wrote: >> While porting >> [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) to Graal, I >> noticed that new CPU features were defined for x86 and AArch64 without being >> exposed via JVMCI. To avoid this problem in future, this PR updates x86 and >> AArch64 to define CPU features with a single macro that is used to generate >> enum declarations as well as vmstructs entries. >> >> In addition, the JVMCI API is updated to exposes the new CPU feature >> constants and now has a check that ensure these constants are in sync with >> the underlying macro definition. > > Doug Simon has updated the pull request incrementally with two additional > commits since the last revision: > > - updated date in copyright > - added blank lines after macros LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v3]
On Mon, 19 Apr 2021 19:38:52 GMT, Doug Simon wrote:
>> src/hotspot/cpu/x86/vmStructs_x86.hpp line 45:
>>
>>> 43:
>>> 44: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit)
>>> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
>>> 45: #define VM_LONG_CPU_FEATURE_CONSTANTS
>>> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
>>
>> Do we need to keep `VM_LONG_CONSTANTS_CPU` after you removed its body here
>> and in vmStructs_jvmci.cpp?
>>
>> What about `VM_INT_CONSTANTS_CPU` here? vmStructs_jvmci.cpp duplicates it.
>
> `vmStructs.cpp` and `vmStructs_jvmci.cpp` are disjoint. This file (i.e.
> `vmStructs_x86.hpp`) is only used by `vmStructs.cpp`.
> `vmStructs.cpp` expects all macros such as `VM_LONG_CONSTANTS_CPU` to be
> defined.
> `vmStructs_jvmci.cpp` will provide a dummy definition for missing macros.
Got it. Even so they are empty everywhere :(
>> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 753:
>>
>>> 751:
>>> 752: #define DECLARE_INT_CPU_FEATURE_CONSTANT(id, name, bit)
>>> GENERATE_VM_INT_CONSTANT_ENTRY(VM_Version::CPU_##id)
>>> 753: #define VM_INT_CPU_FEATURE_CONSTANTS
>>> CPU_FEATURE_FLAGS(DECLARE_INT_CPU_FEATURE_CONSTANT)
>>
>> Missing `#undef DECLARE_INT_CPU_FEATURE_CONSTANT`.
>
> No, it must stay defined up to the point `VM_INT_CPU_FEATURE_CONSTANTS` is
> used. Since this is a `.cpp` file, it's ok to leave it defined.
I see.
>> src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIBackendFactory.java
>> line 66:
>>
>>> 64: long bitMask = e.getValue();
>>> 65: String key = e.getKey();
>>> 66: if (key.startsWith("VM_Version::CPU_")) {
>>
>> As I understand this code, it goes over `constants` values passed from VM
>> and Trying to map them to `enumType`. It catches cases when a value is
>> missing in `enumType`. What about case when `enumType` has `extra` value
>> which is not defined in `constants`?
>
> We could warn about that but cannot remove it without breaking backwards
> capability for JVMCI wrt Graal. Such a deleted capability will simply be seen
> as "not supported" by Graal.
Okay.
-
PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v4]
On Mon, 19 Apr 2021 19:56:45 GMT, Doug Simon wrote: >> While porting >> [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) to Graal, I >> noticed that new CPU features were defined for x86 and AArch64 without being >> exposed via JVMCI. To avoid this problem in future, this PR updates x86 and >> AArch64 to define CPU features with a single macro that is used to generate >> enum declarations as well as vmstructs entries. >> >> In addition, the JVMCI API is updated to exposes the new CPU feature >> constants and now has a check that ensure these constants are in sync with >> the underlying macro definition. > > Doug Simon has updated the pull request incrementally with two additional > commits since the last revision: > > - updated date in copyright > - added blank lines after macros You need review from Runtime group too. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v4]
> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) > to Graal, I noticed that new CPU features were defined for x86 and AArch64 > without being exposed via JVMCI. To avoid this problem in future, this PR > updates x86 and AArch64 to define CPU features with a single macro that is > used to generate enum declarations as well as vmstructs entries. > > In addition, the JVMCI API is updated to exposes the new CPU feature > constants and now has a check that ensure these constants are in sync with > the underlying macro definition. Doug Simon has updated the pull request incrementally with two additional commits since the last revision: - updated date in copyright - added blank lines after macros - Changes: - all: https://git.openjdk.java.net/jdk/pull/3558/files - new: https://git.openjdk.java.net/jdk/pull/3558/files/124019d7..c6be1437 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=02-03 Stats: 11 lines in 10 files changed: 2 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/3558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558 PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v3]
On Mon, 19 Apr 2021 19:00:09 GMT, Vladimir Kozlov wrote:
>> Doug Simon 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 one
>> new commit since the last revision:
>>
>> avoid use of a lambda in JVMCI initialization
>
> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 753:
>
>> 751:
>> 752: #define DECLARE_INT_CPU_FEATURE_CONSTANT(id, name, bit)
>> GENERATE_VM_INT_CONSTANT_ENTRY(VM_Version::CPU_##id)
>> 753: #define VM_INT_CPU_FEATURE_CONSTANTS
>> CPU_FEATURE_FLAGS(DECLARE_INT_CPU_FEATURE_CONSTANT)
>
> Missing `#undef DECLARE_INT_CPU_FEATURE_CONSTANT`.
No, it must stay defined up to the point `VM_INT_CPU_FEATURE_CONSTANTS` is
used. Since this is a `.cpp` file, it's ok to leave it defined.
> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 769:
>
>> 767:
>> 768: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit)
>> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
>> 769: #define VM_LONG_CPU_FEATURE_CONSTANTS
>> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
>
> Missing `#undef DECLARE_LONG_CPU_FEATURE_CONSTANT`.
So comment as for `DECLARE_INT_CPU_FEATURE_CONSTANT`.
> src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIBackendFactory.java
> line 66:
>
>> 64: long bitMask = e.getValue();
>> 65: String key = e.getKey();
>> 66: if (key.startsWith("VM_Version::CPU_")) {
>
> As I understand this code, it goes over `constants` values passed from VM and
> Trying to map them to `enumType`. It catches cases when a value is missing in
> `enumType`. What about case when `enumType` has `extra` value which is not
> defined in `constants`?
We could warn about that but cannot remove it without breaking backwards
capability for JVMCI wrt Graal. Such a deleted capability will simply be seen
as "not supported" by Graal.
-
PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v3]
On Mon, 19 Apr 2021 18:58:46 GMT, Vladimir Kozlov wrote: >> Doug Simon 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 one >> new commit since the last revision: >> >> avoid use of a lambda in JVMCI initialization > > src/hotspot/cpu/x86/vmStructs_x86.hpp line 45: > >> 43: >> 44: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit) >> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id) >> 45: #define VM_LONG_CPU_FEATURE_CONSTANTS >> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT) > > Do we need to keep `VM_LONG_CONSTANTS_CPU` after you removed its body here > and in vmStructs_jvmci.cpp? > > What about `VM_INT_CONSTANTS_CPU` here? vmStructs_jvmci.cpp duplicates it. `vmStructs.cpp` and `vmStructs_jvmci.cpp` are disjoint. This file (i.e. `vmStructs_x86.hpp`) is only used by `vmStructs.cpp`. `vmStructs.cpp` expects all macros such as `VM_LONG_CONSTANTS_CPU` to be defined. `vmStructs_jvmci.cpp` will provide a dummy definition for missing macros. - PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v3]
On Mon, 19 Apr 2021 18:57:37 GMT, Vladimir Kozlov wrote: >> Doug Simon 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 one >> new commit since the last revision: >> >> avoid use of a lambda in JVMCI initialization > > src/hotspot/cpu/aarch64/vm_version_aarch64.hpp line 118: > >> 116: decl(STXR_PREFETCH, "stxr_prefetch", 29) \ >> 117: decl(A53MAC,"a53mac",30) >> 118: #define DECLARE_CPU_FEATURE_FLAG(id, name, bit) CPU_##id = (1 << bit), > > Add empty line before to separate `CPU_FEATURE_FLAGS` macro. Ok. - PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v3]
On Mon, 19 Apr 2021 09:46:17 GMT, Doug Simon wrote:
>> While porting
>> [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) to Graal, I
>> noticed that new CPU features were defined for x86 and AArch64 without being
>> exposed via JVMCI. To avoid this problem in future, this PR updates x86 and
>> AArch64 to define CPU features with a single macro that is used to generate
>> enum declarations as well as vmstructs entries.
>>
>> In addition, the JVMCI API is updated to exposes the new CPU feature
>> constants and now has a check that ensure these constants are in sync with
>> the underlying macro definition.
>
> Doug Simon 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 one new
> commit since the last revision:
>
> avoid use of a lambda in JVMCI initialization
Please, update copyright years in files you touched.
src/hotspot/cpu/aarch64/vm_version_aarch64.hpp line 118:
> 116: decl(STXR_PREFETCH, "stxr_prefetch", 29) \
> 117: decl(A53MAC,"a53mac",30)
> 118: #define DECLARE_CPU_FEATURE_FLAG(id, name, bit) CPU_##id = (1 << bit),
Add empty line before to separate `CPU_FEATURE_FLAGS` macro.
src/hotspot/cpu/x86/vmStructs_x86.hpp line 45:
> 43:
> 44: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit)
> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
> 45: #define VM_LONG_CPU_FEATURE_CONSTANTS
> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
Do we need to keep `VM_LONG_CONSTANTS_CPU` after you removed its body here and
in vmStructs_jvmci.cpp?
What about `VM_INT_CONSTANTS_CPU` here? vmStructs_jvmci.cpp duplicates it.
src/hotspot/cpu/x86/vm_version_x86.hpp line 363:
> 361: decl(AVX512_VBMI, "avx512_vbmi", 45) /* Vector BMI
> instructions */ \
> 362: decl(HV,"hv",46) /* Hypervisor
> instructions */
> 363: #define DECLARE_CPU_FEATURE_FLAG(id, name, bit) CPU_##id = (1ULL << bit),
Add empty line before it to separate `CPU_FEATURE_FLAGS` macro more clear.
src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 753:
> 751:
> 752: #define DECLARE_INT_CPU_FEATURE_CONSTANT(id, name, bit)
> GENERATE_VM_INT_CONSTANT_ENTRY(VM_Version::CPU_##id)
> 753: #define VM_INT_CPU_FEATURE_CONSTANTS
> CPU_FEATURE_FLAGS(DECLARE_INT_CPU_FEATURE_CONSTANT)
Missing `#undef DECLARE_INT_CPU_FEATURE_CONSTANT`.
src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 769:
> 767:
> 768: #define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit)
> GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
> 769: #define VM_LONG_CPU_FEATURE_CONSTANTS
> CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
Missing `#undef DECLARE_LONG_CPU_FEATURE_CONSTANT`.
src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIBackendFactory.java
line 66:
> 64: long bitMask = e.getValue();
> 65: String key = e.getKey();
> 66: if (key.startsWith("VM_Version::CPU_")) {
As I understand this code, it goes over `constants` values passed from VM and
Trying to map them to `enumType`. It catches cases when a value is missing in
`enumType`. What about case when `enumType` has `extra` value which is not
defined in `constants`?
-
PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v3]
> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) > to Graal, I noticed that new CPU features were defined for x86 and AArch64 > without being exposed via JVMCI. To avoid this problem in future, this PR > updates x86 and AArch64 to define CPU features with a single macro that is > used to generate enum declarations as well as vmstructs entries. > > In addition, the JVMCI API is updated to exposes the new CPU feature > constants and now has a check that ensure these constants are in sync with > the underlying macro definition. Doug Simon 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 one new commit since the last revision: avoid use of a lambda in JVMCI initialization - Changes: - all: https://git.openjdk.java.net/jdk/pull/3558/files - new: https://git.openjdk.java.net/jdk/pull/3558/files/bdf9eefb..124019d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558 PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features [v2]
> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) > to Graal, I noticed that new CPU features were defined for x86 and AArch64 > without being exposed via JVMCI. To avoid this problem in future, this PR > updates x86 and AArch64 to define CPU features with a single macro that is > used to generate enum declarations as well as vmstructs entries. > > In addition, the JVMCI API is updated to exposes the new CPU feature > constants and now has a check that ensure these constants are in sync with > the underlying macro definition. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: avoid use of a lambda in JVMCI initialization - Changes: - all: https://git.openjdk.java.net/jdk/pull/3558/files - new: https://git.openjdk.java.net/jdk/pull/3558/files/b69675f4..bdf9eefb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=00-01 Stats: 23 lines in 3 files changed: 5 ins; 9 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/3558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558 PR: https://git.openjdk.java.net/jdk/pull/3558
RFR: 8265403: consolidate definition of CPU features
While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) to Graal, I noticed that new CPU features were defined for x86 and AArch64 without being exposed via JVMCI. To avoid this problem in future, this PR updates x86 and AArch64 to define CPU features with a single macro that is used to generate enum declarations as well as vmstructs entries. In addition, the JVMCI API is updated to exposes the new CPU feature constants and now has a check that ensure these constants are in sync with the underlying macro definition. - Commit messages: - 8265403: consolidate definition of CPU features Changes: https://git.openjdk.java.net/jdk/pull/3558/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3558&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265403 Stats: 481 lines in 15 files changed: 144 ins; 230 del; 107 mod Patch: https://git.openjdk.java.net/jdk/pull/3558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558 PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265403: consolidate definition of CPU features
On Sat, 17 Apr 2021 20:18:31 GMT, Doug Simon wrote:
> While porting [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974)
> to Graal, I noticed that new CPU features were defined for x86 and AArch64
> without being exposed via JVMCI. To avoid this problem in future, this PR
> updates x86 and AArch64 to define CPU features with a single macro that is
> used to generate enum declarations as well as vmstructs entries.
>
> In addition, the JVMCI API is updated to exposes the new CPU feature
> constants and now has a check that ensure these constants are in sync with
> the underlying macro definition.
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 198:
> 196: sprintf(buf, "0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model,
> _revision);
> 197: if (_model2) sprintf(buf+strlen(buf), "(0x%03x)", _model2);
> 198: #define ADD_FEATURE_IF_SUPPORTED(id, name, bit) if (_features &
> CPU_##id) strcat(buf, ", " name);
I'm not sure why only some of the supported AArch64 CPU features were being
added to `_features_string` but I assume there's no harm in adding them all.
src/hotspot/cpu/x86/vm_version_x86.hpp line 382:
> 380: static const char* _features_names[];
> 381:
> 382: // NB! When adding new CPU feature detection consider updating
> vmStructs_x86.hpp, vmStructs_jvmci.hpp, and
> VM_Version::get_processor_features().
No need for this comment any more as the derivative declarations are now
automatically kept up to date.
src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot.amd64/src/jdk/vm/ci/hotspot/amd64/AMD64HotSpotJVMCIBackendFactory.java
line 57:
> 55:
> 56: Map constants = config.getStore().getConstants();
> 57: Function nameToFeature = name ->
> name.equals("3DNOW_PREFETCH") ? CPUFeature.AMD_3DNOW_PREFETCH :
> CPUFeature.valueOf(name);
The `AMD_3DNOW_PREFETCH` enum constant has to keep its old name to preserve
backward compatibility.
-
PR: https://git.openjdk.java.net/jdk/pull/3558
