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

Schmidt, Lutz lutz.schmidt at sap.com
Thu Jan 24 11:11:37 UTC 2019

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. 


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,
    -----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,

More information about the hotspot-compiler-dev mailing list