RFR(M): 8217459: [PPC64] Cleanup non-vector version of CRC32

[Lindenmaier, Goetz]

Hi Martin,

The change looks good to me. 

Best regards,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Thursday, January 24, 2019 1:12 PM
> To: Schmidt, Lutz <lutz.schmidt at sap.com>; Gustavo Romero
> <gromero at linux.vnet.ibm.com>; 'hotspot-compiler-dev at openjdk.java.net'
> <hotspot-compiler-dev at openjdk.java.net>
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Subject: RE: RFR(M): 8217459: [PPC64] Cleanup non-vector version of CRC32
> 
> Hi Lutz and Gustavo,
> 
> that's fine. Removed the comments which refer to java.util.zip.CRC32 stuff.
> 
> And while reading through the comments, I found out that
> kernel_crc32_singleByte is not useful (since we have the ...Reg version). So I
> just removed it and replaced its only usage by better code
> (TemplateInterpreterGenerator::generate_CRC32_update_entry).
> 
> New webrev:
> http://cr.openjdk.java.net/~mdoerr/8217459_ppc64_crc_consts/webrev.01/
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Schmidt, Lutz
> Sent: Donnerstag, 24. Januar 2019 12:12
> To: Doerr, Martin <martin.doerr at sap.com>; Gustavo Romero
> <gromero at linux.vnet.ibm.com>; 'hotspot-compiler-dev at openjdk.java.net'
> <hotspot-compiler-dev at openjdk.java.net>
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Subject: Re: RFR(M): 8217459: [PPC64] Cleanup non-vector version of CRC32
> 
> Gustavo, Martin,
> 
> I agree, that comment appears somewhat disconnected from the code.
> I'm really not sure if it will help a lot in the future to have a
> grep string that helps finding the related code in java.util.zip.CRC32.
> 
> In short: change it to something meaningful in the local context.
> 
> Thanks,
> Lutz
> 
> On 24.01.19, 11:02, "Doerr, Martin" <martin.doerr at sap.com> wrote:
> 
>     Hi Gustavo,
> 
>     thank you for reviewing and testing.
> 
>     Seems like many comments were taken from java.util.zip.CRC32. I guess it
> was intended to refer to it.
>     I think it's not bad to have it this way because it makes it easier to compare
> both implementations.
>     Maybe Lutz can comment on this and if he would like to keep it this way.
> 
>     Best regards,
>     Martin
> 
> 
>     -----Original Message-----
>     From: Gustavo Romero <gromero at linux.vnet.ibm.com>
>     Sent: Mittwoch, 23. Januar 2019 23:18
>     To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
>     Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>     Subject: Re: RFR(M): 8217459: [PPC64] Cleanup non-vector version of
> CRC32
> 
>     Hi Martin,
> 
>     On 01/21/2019 04:07 PM, Doerr, Martin wrote:
>     > PPC64 currently contains static tables for CRC32/CRC32C calculations. We
> only need some of them depending on Endianess and on whether vector
> instructions are available or not.
>     > We can get rid of quite some code when we generate these constants at
> startup as we already do for the vector version.
>     > In addition, we can save one register in the vector case because we can
> use one constants pointer for all related constants.
>     > Webrev:
>     >
> http://cr.openjdk.java.net/~mdoerr/8217459_ppc64_crc_consts/webrev.00/
> <http://cr.openjdk.java.net/%7Emdoerr/8217459_ppc64_crc_consts/webre
> v.00/>
> 
>     Thanks for the clean-up. Change looks good!
> 
>     It's good to see fold_8bit_crc32 and kernel_crc32_1byte going away (I just
>     noted them recently so I missed both in my previous clean-up). And also
>     the static table simplification.
> 
>     I tested the change with different array sizes and byte values with and
>     without vpmsum in the CPU, i.e. has_vpmsumb() = false, and found no
> issues.
> 
>     Only a nit: should we update the following comment and replace
> 'timesXtoThe32'
>     by something better, maybe 'table'? That name doesn't look much
> meaningful in the
>     current context and seems taken from the native code for
> java.util.zip.CRC32:
> 
>     3902 /**
>     3903  * uint32_t crc;
>     3904  * timesXtoThe32[crc & 0xFF] ^ (crc >> 8);
>     3905  */
>     3906 void MacroAssembler::fold_byte_crc32(Register crc, Register val,
> Register table, Register tmp) {
> 
> 
>     Best regards,
>     Gustavo
> 
> 
> 
>