RFR (M): 8134553: CRC32C implementations for Nehalem x86/amd64 & Westmere+ x86/amd64

[Vladimir Kozlov]

Nice work Tomek.

Few things.

Why there are no C1 changes?

templateInterpreter_x86_64.cpp has strange symbols in comments:

Java� Virtual
type�long�and�double�have

Code style comments:

If possible can you not use namespace? You can have named enums and 
local static methods instead.

And macroAssembler_x86.cpp totally goes against our code style.
I would suggest to move IPL_Alg4 and other local methods into 
MacroAssembler class - 'This->' looks strange here.
I would also rename all these local methods to have prefix crc32c_.

Also align parameters lines to (:

+  void IPL_Alg4(INOUT(Register B), uint32_t n,
+                Scratch(Register C), Scratch(Register D), 
Scratch(Register Z),
+                MacroAssembler * This) {


+  CRC32C::ProcChunk(CRC32C::HIGH, CONSTOrPreCompConstIndex[0], 
CONSTOrPreCompConstIndex[1], IsPclmulqdqSupported,
+                   len, buf, crc,
+                   A, B, C,
+                   AXMM, BXMM, CXMM,
+                   D, E, F,
+                   this);

Instead of Scratch(Register C) use Register tmp1.
I am fine with IN, INOUT, OUT, Scratch macros but Nehalem() and 
Westmere() is strange to see here - I do not understand their meaning.

rename CONSTOrPreCompConstInde* to use low case and _.

Argument name: IsPclmulqdqSupported --> is_pclmulqdq_supported

stubRoutines_x86.cpp upper case name are used mostly for constants and 
enums. You have array PCLMULQDQ[] (I would change PCLMULQDQ name too - 
pclmulqdq_table) and next weird definition:

+  #undef CONST
+  static juint x;
+  #define CONST x

+      CONST = PowN[j];

Keep it local as X_CONST if you want to highlight that it is constant 
for following expression. Simple 'CONST' name is confusing.

Rename  crc32c_IPL_Alg2Alt2Fast --> crc32c_IPL_alg2 (to you need alt fast?).

Rename GenerateCRC32CTable --> generate_CRC32C_table

crc32c.h:

  // a) constants table generation 
(C:\Java\jdk9hs-comp\hotspot\src\cpu\x86\vm\stubRoutines_x86.cpp)
   62

Use hotspot/src/cpu/x86/vm/stubRoutines_x86.cpp

Thanks,
Vladimir

On 8/27/15 11:47 AM, Wojtowicz, Tomasz wrote:
> I would like to contribute following change:
>
> Review details
>
> Review Title:      CRC32C implementations for Nehalem x86/amd64 &
> Westmere+ x86/amd64
>
> Review ID:          #8134553
>
> Diff: http://cr.openjdk.java.net/~mcberg/8134553/webrev.01/
> <http://cr.openjdk.java.net/%7Emcberg/8134553/webrev.01/>
>
> Description:        Efficient use of a crc32 hardware instruction by
> division of a problem to a predefined chunks of an increasing size and
> further by 3 to be computed hiding instruction latencies. x86 delivers
> up to 8x improvement vs. java library, amd64 stops at even more -> 16x.
>
>                                  Performance data are attached to this
> message for your convenience.
>
>                                  No regressions has been observed on
> hotspot/compiler x86_64.
>
> Link: https://bugs.openjdk.java.net/browse/JDK-8134553
>
> Author:                Tomasz, Wojtowicz
>
> --
>
> Thank you,
>
> Tomek
>