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

[Gustavo Romero]

Hi Martin,

On 01/24/2019 10:11 AM, Doerr, Martin wrote:
> 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/

Thanks for the updated webrev.

The additional clean-up makes the code easier to read/follow too.

LGTM.

Best regards,
Gustavo

> 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/webrev.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
>      
>      
> 
>