[PATCH v3] JDK-8222090: Add Hygon Dhyana support
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Apr 12 00:11:18 UTC 2019
Good.
Thanks,
Vladimir
On 4/11/19 4:29 PM, David Holmes wrote:
> Hi Vladimir,
>
> On 12/04/2019 2:04 am, Vladimir Kozlov wrote:
>> Hi David,
>>
>> Thank you for cleaning up code. Looks good except next small issues:
>>
>> You missed check in assembler_x86.cpp.
>
> Fixed:
>
> diff -r 4eefc9f3313c src/hotspot/cpu/x86/assembler_x86.cpp
> --- a/src/hotspot/cpu/x86/assembler_x86.cpp
> +++ b/src/hotspot/cpu/x86/assembler_x86.cpp
> @@ -3099,7 +3099,7 @@
> }
> return;
> }
> - if (UseAddressNop && VM_Version::is_amd()) {
> + if (UseAddressNop && VM_Version::is_amd_family()) {
>
>
>> In vm_version_ext_x86.cpp indent is wrong in 2 cases: 4 spaces are used instead of 2.
>
> Will fix before pushing and update webrev .v4 in place.
>
> Thanks,
> David
>
>> Thanks,
>> Vladimir
>>
>> On 4/10/19 10:26 PM, David Holmes wrote:
>>> Updated webrev:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8222090/webrev.v4/
>>>
>>> Seems comments below
>>>
>>> On 11/04/2019 12:58 pm, Jinke Fan wrote:
>>>> On 2019/4/11 10:48, Vladimir Kozlov wrote:
>>>>> On 4/10/19 6:14 PM, David Holmes wrote:
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> Thanks for taking look at this.
>>>>>>
>>>>>> On 11/04/2019 2:41 am, Vladimir Kozlov wrote:
>>>>>>> Instead of repeated is_amd() || is_hygon() you can use is_amd_or_hygon() to be more clean.
>>>>>>
>>>>>> That doesn't really scale if we get more "cousins" joining the AMD family. I prefer to see the explicit
>>>>>> disjunction form.
>>>>>
>>>>> is_amd_family() ? If it was 2 or 3 cases I would agree with you. But you have 10 checks!
>>>
>>> I made this change. It is also "forced" me to fix up numerous coding style nits with missing and extra spaces in
>>> if-expressions. (Sorry)
>>>
>>>>>
>>>>>>
>>>>>>> Next check should be explicit for CPU:
>>>>>>>
>>>>>>> + // Some defaults for AMD family 17h || Hygon family 18h
>>>>>>> + if ( cpu_family() == 0x17 || cpu_family() == 0x18 ) {
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> + // Some defaults for AMD family 17h || Hygon family 18h
>>>>>>> + if ( (cpu_family() == 0x17) && is_amd() || (cpu_family() == 0x18) && is_hygon()) {
>>>>>>
>>>>>> I'm wondering if that should just be an assertion? I would not expect it to be possible to have a family 0x17 that
>>>>>> is not AMD and likewise for family 0x18 and Hygon. But then I don't know how these family ids get issued. ??
>>>>>
>>>>> It is not clear what agreement AMD has with these companies. Any company (as AMD and Intel) may have own set of
>>>>> cpu_family ids and we can't assume that ids will not be reused. They have only 8 bits for ID.
>>>> Hygon has negotiated with AMD to make sure that only Hygon will
>>>> use family 18h, so try to minimize code modification and share most
>>>> codes with AMD under this consideration.
>>>
>>> I think in that case we do not need to also check for is_amd() and is_hygon() (noting that the code in question is
>>> already in a block guarded by is_amd_family()).
>>>
>>> Thanks,
>>> David
>>> -----
>>>>
>>>> Best Regards！
>>>> Fanjinke
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/10/19 12:33 AM, David Holmes wrote:
>>>>>>>> Hi Fanjinke,
>>>>>>>>
>>>>>>>> This looks fine to me. I've updated the webrev at:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~dholmes/8222090/webrev.v3/
>>>>>>>>
>>>>>>>> We need a second reviewer before I push it.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>> On 9/04/2019 5:22 pm, Jinke Fan wrote:
>>>>>>>>> diff --git a/src/hotspot/cpu/x86/assembler_x86.cpp b/src/hotspot/cpu/x86/assembler_x86.cpp
>>>>>>>>> --- a/src/hotspot/cpu/x86/assembler_x86.cpp
>>>>>>>>> +++ b/src/hotspot/cpu/x86/assembler_x86.cpp
>>>>>>>>> @@ -3099,7 +3099,7 @@
>>>>>>>>> }
>>>>>>>>> return;
>>>>>>>>> }
>>>>>>>>> - if (UseAddressNop && VM_Version::is_amd()) {
>>>>>>>>> + if (UseAddressNop && (VM_Version::is_amd() || VM_Version::is_hygon())) {
>>>>>>>>> //
>>>>>>>>> // Using multi-bytes nops "0x0F 0x1F [address]" for AMD.
>>>>>>>>> // 1: 0x90
>>>>>>>>> diff --git a/src/hotspot/cpu/x86/vm_version_ext_x86.cpp b/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>>>>>>> --- a/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>>>>>>> +++ b/src/hotspot/cpu/x86/vm_version_ext_x86.cpp
>>>>>>>>> @@ -340,6 +340,10 @@
>>>>>>>>> return !is_amd_Barcelona();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> + if (is_hygon()) {
>>>>>>>>> + return true;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> return false;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> @@ -407,6 +411,10 @@
>>>>>>>>> }
>>>>>>>>> return _family_id_intel[cpu_family_id];
>>>>>>>>> }
>>>>>>>>> + if (is_hygon()) {
>>>>>>>>> + return "Dhyana";
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> return "Unknown x86";
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> @@ -423,6 +431,9 @@
>>>>>>>>> } else if (is_amd()) {
>>>>>>>>> cpu_type = "AMD";
>>>>>>>>> x64 = cpu_is_em64t() ? " AMD64" : "";
>>>>>>>>> + } else if (is_hygon()) {
>>>>>>>>> + cpu_type = "Hygon";
>>>>>>>>> + x64 = cpu_is_em64t() ? " AMD64" : "";
>>>>>>>>> } else {
>>>>>>>>> cpu_type = "Unknown x86";
>>>>>>>>> x64 = cpu_is_em64t() ? " x86_64" : "";
>>>>>>>>> diff --git a/src/hotspot/cpu/x86/vm_version_x86.cpp b/src/hotspot/cpu/x86/vm_version_x86.cpp
>>>>>>>>> --- a/src/hotspot/cpu/x86/vm_version_x86.cpp
>>>>>>>>> +++ b/src/hotspot/cpu/x86/vm_version_x86.cpp
>>>>>>>>> @@ -1165,7 +1165,7 @@
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> - if( is_amd() ) { // AMD cpus specific settings
>>>>>>>>> + if( is_amd() || is_hygon() ) { // AMD cpus specific settings
>>>>>>>>> if( supports_sse2() && FLAG_IS_DEFAULT(UseAddressNop) ) {
>>>>>>>>> // Use it on new AMD cpus starting from Opteron.
>>>>>>>>> UseAddressNop = true;
>>>>>>>>> @@ -1239,8 +1239,8 @@
>>>>>>>>> }
>>>>>>>>> #endif // COMPILER2
>>>>>>>>>
>>>>>>>>> - // Some defaults for AMD family 17h
>>>>>>>>> - if ( cpu_family() == 0x17 ) {
>>>>>>>>> + // Some defaults for AMD family 17h || Hygon family 18h
>>>>>>>>> + if ( cpu_family() == 0x17 || cpu_family() == 0x18 ) {
>>>>>>>>> // On family 17h processors use XMM and UnalignedLoadStores for Array Copy
>>>>>>>>> if (supports_sse2() && FLAG_IS_DEFAULT(UseXMMForArrayCopy)) {
>>>>>>>>> FLAG_SET_DEFAULT(UseXMMForArrayCopy, true);
>>>>>>>>> diff --git a/src/hotspot/cpu/x86/vm_version_x86.hpp b/src/hotspot/cpu/x86/vm_version_x86.hpp
>>>>>>>>> --- a/src/hotspot/cpu/x86/vm_version_x86.hpp
>>>>>>>>> +++ b/src/hotspot/cpu/x86/vm_version_x86.hpp
>>>>>>>>> @@ -495,13 +495,13 @@
>>>>>>>>> result |= CPU_CX8;
>>>>>>>>> if (_cpuid_info.std_cpuid1_edx.bits.cmov != 0)
>>>>>>>>> result |= CPU_CMOV;
>>>>>>>>> - if (_cpuid_info.std_cpuid1_edx.bits.fxsr != 0 || (is_amd() &&
>>>>>>>>> + if (_cpuid_info.std_cpuid1_edx.bits.fxsr != 0 || ((is_amd() || is_hygon()) &&
>>>>>>>>> _cpuid_info.ext_cpuid1_edx.bits.fxsr != 0))
>>>>>>>>> result |= CPU_FXSR;
>>>>>>>>> // HT flag is set for multi-core processors also.
>>>>>>>>> if (threads_per_core() > 1)
>>>>>>>>> result |= CPU_HT;
>>>>>>>>> - if (_cpuid_info.std_cpuid1_edx.bits.mmx != 0 || (is_amd() &&
>>>>>>>>> + if (_cpuid_info.std_cpuid1_edx.bits.mmx != 0 || ((is_amd() || is_hygon()) &&
>>>>>>>>> _cpuid_info.ext_cpuid1_edx.bits.mmx != 0))
>>>>>>>>> result |= CPU_MMX;
>>>>>>>>> if (_cpuid_info.std_cpuid1_edx.bits.sse != 0)
>>>>>>>>> @@ -576,8 +576,8 @@
>>>>>>>>> if (_cpuid_info.std_cpuid1_ecx.bits.fma != 0)
>>>>>>>>> result |= CPU_FMA;
>>>>>>>>>
>>>>>>>>> - // AMD features.
>>>>>>>>> - if (is_amd()) {
>>>>>>>>> + // AMD|Hygon features.
>>>>>>>>> + if (is_amd() || is_hygon()) {
>>>>>>>>> if ((_cpuid_info.ext_cpuid1_edx.bits.tdnow != 0) ||
>>>>>>>>> (_cpuid_info.ext_cpuid1_ecx.bits.prefetchw != 0))
>>>>>>>>> result |= CPU_3DNOW_PREFETCH;
>>>>>>>>> @@ -711,6 +711,7 @@
>>>>>>>>> static int cpu_family() { return _cpu;}
>>>>>>>>> static bool is_P6() { return cpu_family() >= 6; }
>>>>>>>>> static bool is_amd() { assert_is_initialized(); return _cpuid_info.std_vendor_name_0 ==
>>>>>>>>> 0x68747541; } // 'htuA'
>>>>>>>>> + static bool is_hygon() { assert_is_initialized(); return _cpuid_info.std_vendor_name_0 ==
>>>>>>>>> 0x6F677948; } // 'ogyH'
>>>>>>>>> static bool is_intel() { assert_is_initialized(); return _cpuid_info.std_vendor_name_0 ==
>>>>>>>>> 0x756e6547; } // 'uneG'
>>>>>>>>> static bool is_zx() { assert_is_initialized(); return (_cpuid_info.std_vendor_name_0 ==
>>>>>>>>> 0x746e6543) || (_cpuid_info.std_vendor_name_0 == 0x68532020); } // 'tneC'||'hS '
>>>>>>>>> static bool is_atom_family() { return ((cpu_family() == 0x06) && ((extended_cpu_model() == 0x36) ||
>>>>>>>>> (extended_cpu_model() == 0x37) || (extended_cpu_model() == 0x4D))); } //Silvermont and Centerton
>>>>>>>>> @@ -734,7 +735,7 @@
>>>>>>>>> if (!supports_topology || result == 0) {
>>>>>>>>> result = (_cpuid_info.dcp_cpuid4_eax.bits.cores_per_cpu + 1);
>>>>>>>>> }
>>>>>>>>> - } else if (is_amd()) {
>>>>>>>>> + } else if (is_amd() || is_hygon()) {
>>>>>>>>> result = (_cpuid_info.ext_cpuid8_ecx.bits.cores_per_cpu + 1);
>>>>>>>>> } else if (is_zx()) {
>>>>>>>>> bool supports_topology = supports_processor_topology();
>>>>>>>>> @@ -770,7 +771,7 @@
>>>>>>>>> intx result = 0;
>>>>>>>>> if (is_intel()) {
>>>>>>>>> result = (_cpuid_info.dcp_cpuid4_ebx.bits.L1_line_size + 1);
>>>>>>>>> - } else if (is_amd()) {
>>>>>>>>> + } else if (is_amd() || is_hygon()) {
>>>>>>>>> result = _cpuid_info.ext_cpuid5_ecx.bits.L1_line_size;
>>>>>>>>> } else if (is_zx()) {
>>>>>>>>> result = (_cpuid_info.dcp_cpuid4_ebx.bits.L1_line_size + 1);
>>>>>>>>> @@ -857,7 +858,7 @@
>>>>>>>>>
>>>>>>>>> // AMD features
>>>>>>>>> static bool supports_3dnow_prefetch() { return (_features & CPU_3DNOW_PREFETCH) != 0; }
>>>>>>>>> - static bool supports_mmx_ext() { return is_amd() && _cpuid_info.ext_cpuid1_edx.bits.mmx_amd != 0; }
>>>>>>>>> + static bool supports_mmx_ext() { return (is_amd()||is_hygon()) && _cpuid_info.ext_cpuid1_edx.bits.mmx_amd
>>>>>>>>> != 0; }
>>>>>>>>> static bool supports_lzcnt() { return (_features & CPU_LZCNT) != 0; }
>>>>>>>>> static bool supports_sse4a() { return (_features & CPU_SSE4A) != 0; }
>>>>>>>>>
>>>>>>>>> @@ -870,7 +871,7 @@
>>>>>>>>> }
>>>>>>>>> static bool supports_tscinv() {
>>>>>>>>> return supports_tscinv_bit() &&
>>>>>>>>> - ( (is_amd() && !is_amd_Barcelona()) ||
>>>>>>>>> + ( ((is_amd()||is_hygon()) && !is_amd_Barcelona()) ||
>>>>>>>>> is_intel_tsc_synched_at_init() );
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> @@ -896,7 +897,7 @@
>>>>>>>>> // Core - 256 / prefetchnta
>>>>>>>>> // It will be used only when AllocatePrefetchStyle > 0
>>>>>>>>>
>>>>>>>>> - if (is_amd()) { // AMD
>>>>>>>>> + if (is_amd() || is_hygon()) { // AMD
>>>>>>>>> if (supports_sse2()) {
>>>>>>>>> return 256; // Opteron
>>>>>>>>> } else {
>>>>>
>>>>
More information about the hotspot-dev
mailing list