[PATCH v3] JDK-8222090: Add Hygon Dhyana support
Jinke Fan
fanjinke51 at yeah.net
Thu Apr 11 05:51:22 UTC 2019
Hi David,
This looks good to me.
Thank all of you.
Best Regards!
Fanjinke
On 2019/4/11 13:26, 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