[PATCH v3] JDK-8222090: Add Hygon Dhyana support
David Holmes
david.holmes at oracle.com
Thu Apr 11 23:29:19 UTC 2019
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