RFR(m) 8164920: ppc: enhancement of CRC32 intrinsic

[Lindenmaier, Goetz]

Hi Hiroshi,

I had a look at your change. While I can't tell whether the algorithm is correct,
I can state all our tests are green.  Spilling the registers to stack is fine as there
is the ShadowZone on the stack.

I would appreciate if it would be ported for big endian, too, as a follow up
though.

Reviewed.

Best regards,
  Goetz




> -----Original Message-----
> From: Hiroshi H Horii [mailto:HORII at jp.ibm.com]
> Sent: Dienstag, 20. September 2016 19:03
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: Gustavo Bueno Romero <gromero at br.ibm.com>; hotspot-compiler-
> dev at openjdk.java.net; Volker Simonis (volker.simonis at gmail.com)
> <volker.simonis at gmail.com>; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>
> Subject: RE: RFR(m) 8164920: ppc: enhancement of CRC32 intrinsic
> 
> Hi all,
> 
> Martin thankfully created a webrev with some good correction.
> http://cr.openjdk.java.net/~mdoerr/8164920_ppc_crc32/webrev.01/
> <http://cr.openjdk.java.net/~mdoerr/8164920_ppc_crc32/webrev.01/>
> 
> Could someone review this change again?
> 
> Regards,
> Hiroshi
> -----------------------
> Hiroshi Horii, Ph.D.
> IBM Research - Tokyo
> 
> 
> 
> 
> From:        Hiroshi H Horii/Japan/IBM
> To:        "Doerr, Martin" <martin.doerr at sap.com>
> Cc:        Gustavo Bueno Romero <gromero at br.ibm.com>, "hotspot-compiler-
> dev at openjdk.java.net" <hotspot-compiler-dev at openjdk.java.net>,
> "Volker Simonis (volker.simonis at gmail.com)" <volker.simonis at gmail.com>
> Date:        09/19/2016 02:36
> Subject:        RE: RFR(m) 8164920: ppc: enhancement of CRC32 intrinsic
> 
> ________________________________
> 
> 
> 
> Hi Martin, and all
> (Please allow me to send this mail twice. The first mail is awaiting because it
> exceeded 100KB)
> 
> Thank you for your reviewing. Gustavo and I recreated a new change based
> on your comments. I would like to request a review again.
> 
> My account of cr server is not available now (because of my mistake...) and
> Gustavo cannot create a webrev file with another reason. I would like to
> attach a diff file created with "hg diff -g" in hotspot.  If possible, could
> someone create a webrev file with this changeset?
> 
> [attachment "hotspot.crc32.20160918.changeset" deleted by Hiroshi H
> Horii/Japan/IBM]
> 
> Regards,
> Hiroshi
> -----------------------
> Hiroshi Horii, Ph.D.
> IBM Research - Tokyo
> 
> "Doerr, Martin" <martin.doerr at sap.com> wrote on 09/13/2016 18:35:09:
> 
> > From: "Doerr, Martin" <martin.doerr at sap.com>
> > To: Hiroshi H Horii/Japan/IBM at IBMJP, "hotspot-compiler-
> > dev at openjdk.java.net" <hotspot-compiler-dev at openjdk.java.net>
> > Cc: "Volker Simonis (volker.simonis at gmail.com)"
> > <volker.simonis at gmail.com>, Gustavo Bueno Romero
> <gromero at br.ibm.com>
> > Date: 09/13/2016 18:36
> > Subject: RE: RFR(m) 8164920: ppc: enhancement of CRC32 intrinsic
> >
> > Hi Hiroshi,
> >
> > we appreciate your change. Thanks for contributing it.
> > It basically looks good, but I’d like to propose some minor improvements.
> >
> >
> > kernel_crc32_1word_vpmsumd:
> >
> > 1. The Pre-align code can be implemented shorter:
> >     clrldi_(prealign, buf, 57);
> >     beq(CCR0, L_alignHead);
> >
> >     subfic(prealign, prealign, 128);
> >
> > 2. I'd prefer the label name “L_alignedHead”.
> >
> > 3. The branch b(L_alignTail) and the label are not needed and should
> > get removed.
> >
> >
> > kernel_crc32_1word_aligned:
> >
> > 1. When saving and restoring non-volatile vector register, please
> > use offset differences of -16 instead of -32.
> > (The ABI allows up to 288 bytes to be used in frameless functions so
> > it will fit if -16 is used.)
> >
> > 2. The std instructions should better be used with int offsets so
> > you can get rid of the addi(offset, offset, -8) instructions.
> >
> >
> > Comments:
> > For single line comments "//" should be used instead of "/*". Would
> > be nice if you could change them.
> >
> >
> > Thanks and best regards,
> > Martin
> >
> >
> > From: Hiroshi H Horii [mailto:HORII at jp.ibm.com
> <mailto:HORII at jp.ibm.com> ]
> > Sent: Dienstag, 6. September 2016 16:50
> > To: hotspot-compiler-dev at openjdk.java.net; vladimir.kozlov at oracle.com
> > Cc: Volker Simonis (volker.simonis at gmail.com)
> > <volker.simonis at gmail.com>; Doerr, Martin <martin.doerr at sap.com>;
> > Gustavo Bueno Romero <gromero at br.ibm.com>
> > Subject: RFR(m) 8164920: ppc: enhancement of CRC32 intrinsic
> >
> > Dear Vladimir and all:
> >
> > Can I please request reviews for the following change?
> >
> > JIRA: https://bugs.openjdk.java.net/browse/JDK-8164920
> <https://bugs.openjdk.java.net/browse/JDK-8164920>
> > webrev: http://cr.openjdk.java.net/~gromero/8164920/01/
> <http://cr.openjdk.java.net/~gromero/8164920/01/>
> >
> > As Volker's comments in the above JIRA, this is a ppc64-only
> > improvement which will not
> > affect any of the Oracle platforms in any way.
> >
> > This change includes new implementation of CRC32 Intrinsics for ppc64le.
> > In my local experiment, CRC32 of 64KB was calculated more than 20
> > times faster than original.
> > Performance of CRC32 Intrinsic is important to run recent Apache
> Cassandra.
> > A Cassandra daemon needs to read 64KB data from a disk with CRC32
> > checksum by default.
> >
> > This JIRA entry has "jdk9-fc-request" label.
> > If there is a chance to include new change in JDK 9 for ppc64le, I
> > would like to request
> > a review for this change.
> >
> > Regards,
> > Hiroshi
> > -----------------------
> > Hiroshi Horii, Ph.D.
> > IBM Research - Tokyo
> 
>