Re: 答复: [patch] support zhaoxin x86 cpu vendor ids CentaurHauls and Shanghai

[David Holmes]

On 3/01/2018 6:32 PM, Vic Wang(BJ-RD) wrote:
> Hi David:
> I generate a patch using "hg diff". Please check it again. Thanks very much.

Thanks.

> diff -r 4d28288c9f9e src/hotspot/cpu/x86/assembler_x86.cpp
> --- a/src/hotspot/cpu/x86/assembler_x86.cppThu Dec 07 15:52:46 2017 +0100

Something stripped out the spaces between the file name and timestamp.

I've hosted a webrev for the patch at:

http://cr.openjdk.java.net/~dholmes/8194279/webrev


A few minor comments. I can't comment on the technical accuracy of course :)

All copyright notices need the second year updated to 2018.

- src/hotspot/cpu/x86/assembler_x86.cpp

Seems fine - you copied the existing pattern so I won't call out any 
style bugs. :)

---

- src/hotspot/cpu/x86/vm_version_x86.cpp

633 UseAVX = 0;

Indention is missing.

General style nit: your if statements are very inconsistently formatted. 
A statement like:

if( is_zx() )

should be written:

if (is_zx())

- space after the "if" and no space after ( or before )


In this block:

+     if (is_zx() && ((cpu_family() == 6)||(cpu_family() == 7)) && 
supports_sse3()) {
+ #ifdef COMPILER2
+     if (FLAG_IS_DEFAULT(UseFPUForSpilling) && supports_sse4_2()) {
+       FLAG_SET_DEFAULT(UseFPUForSpilling, true);
+     }
+ #endif

I see this copies the existing pattern but isn't it simpler just to write:

     if (is_zx() && ((cpu_family() == 6)||(cpu_family() == 7)) && 
supports_sse4_2()) {
#ifdef COMPILER2
     if (FLAG_IS_DEFAULT(UseFPUForSpilling)) {
       FLAG_SET_DEFAULT(UseFPUForSpilling, true);
     }
#endif

?

---

-  src/hotspot/cpu/x86/vm_version_x86.hpp

310 CPU_FAMILY_ZX_CORE_F7    = 7,

Indentation is missing.

555 // ZX features.

Indentation is missing.

  556     if(is_zx()) {
  557       if(_cpuid_info.ext_cpuid1_ecx.bits.lzcnt_intel != 0)
  558         result |= CPU_LZCNT;
  559       // for ZX, ecx.bits.misalignsse bit (bit 8) indicates 
support for prefetchw
  560       if (_cpuid_info.ext_cpuid1_ecx.bits.misalignsse != 0) {
  561         result |= CPU_3DNOW_PREFETCH;
  562       }
  563     }

The "intel" block you copied is full of style issues :( But as it is 
exactly the same can't you just adjust:

546     if(is_intel()) {

to

546     if (is_intel() || is_zx()) {

? Similarly for cores_per_cpu(), threads_per_core() and L1_line_size().

Otherwise there are indentation issues in your changes to all those 
functions.

Thanks,
David
-----