RFR (M) 8175368: [s390] Provide intrinsic implementation for CRC32C

[Doerr, Martin]

Hi Lutz,

thanks for implementing my suggestions and for the explanations. The change looks good to me.

Best regards,
Martin


-----Original Message-----
From: Schmidt, Lutz 
Sent: Dienstag, 28. Februar 2017 15:28
To: Doerr, Martin <martin.doerr at sap.com>; Vladimir Kozlov <vladimir.kozlov at oracle.com>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR (M) 8175368: [s390] Provide intrinsic implementation for CRC32C

Hi Martin, 

Thank you for reviewing and for your suggestions. I have reworked my changes to reflect your improvements. A new webrev can be found here: http://cr.openjdk.java.net/~lucy/webrevs/8175368.01/

The lookup tables in stubRoutines_s390.cpp have been verified by calculating CRC32C values for various byte streams with and without using the tables. Because the tables are auto-generated, it is very unlikely that just a few individual table elements are incorrect.

Best regards,
Lutz



On 28.02.17, 11:57, "Doerr, Martin" <martin.doerr at sap.com> wrote:

    Hi Lutz,
    
    thank you very much for implementing it. It looks good. I especially like that you share a lot of the code between CRC32 and CRC32C.
    I only have some minor suggestions:
    
    c1_LIRGenerator_s390.cpp
    load_int_as_long was implemented for stubs which expect sign (or zero) extended integer parameters.
    As C2 was changed to no longer perform int to long conversions for stub calls, I think this should better be removed from C1, too.
    The stubs are shared between C1 and C2 and they don't need conversions anymore, so I'd appreciate if the load_int_as_long got removed completely.
    
    macroAssembler_s390.cpp
    The function update_1word_crc32 uses separate load and xor instructions. I think some of them should get combined by using xor register,mem.
    
    stubGenerator_s390.cpp
    The comment in generate_CRC32_updateBytes should be: "must be same register as in generate..."
    
    stubRoutines_s390.cpp
    I didn't review all numbers :-), but I guess you have tested well enough?
    
    It'd be nice if you could share some performance numbers.
    
    Thanks and best regards,
    Martin
    
    
    
    -----Original Message-----
    From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Schmidt, Lutz
    Sent: Montag, 27. Februar 2017 22:33
    To: Vladimir Kozlov <vladimir.kozlov at oracle.com>; hotspot-compiler-dev at openjdk.java.net
    Subject: Re: RFR (M) 8175368: [s390] Provide intrinsic implementation for CRC32C
    
    Thank you, Vladimir, for looking at this. 
    
    Regards, Lutz
    
    
    On 27.02.17, 22:30, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
    
        Shared changes are good. I assume s390 code will be reviewed by experts.
        
        Thanks,
        Vladimir
        
        On 2/27/17 9:34 AM, Schmidt, Lutz wrote:
        > Hi all,
        >
        >
        >
        > the webrev URL below was incorrect. It displayed the correct location,
        > but pointed elsewhere. The issue is fixed now and the URLs are correct.
        >
        >
        >
        > Thank you!
        >
        > Lutz
        >
        >
        >
        > *From: *Lutz Schmidt <lutz.schmidt at sap.com>
        > *Date: *Montag, 27. Februar 2017 um 15:55
        > *To: *"hotspot-compiler-dev at openjdk.java.net"
        > <hotspot-compiler-dev at openjdk.java.net>
        > *Subject: *RFR (M) 8175368: [s390] Provide intrinsic implementation for
        > CRC32C
        >
        >
        >
        > Hi all,
        >
        >
        >
        > may I kindly request reviews for my medium enhancement? Further down the
        > road, I would need a sponsor, too.
        >
        >
        >
        > Bug: https://bugs.openjdk.java.net/browse/JDK-8175368
        >
        > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8175368/
        >
        >
        >
        > Description:
        >
        > This intrinsic implementation provides some performance benefit over the
        > standard Java implementation. It uses only well-known “standard”
        > instructions, available on all supported IBM System z hardware. Being
        > very similar to the CRC32 intrinsics, it was tried to share as much code
        > as possible between these two.
        >
        >
        >
        > Performance may be expected to increase up to 2.0x, compared to a run
        > with –XX:-UseCRC32CIntrinsics. Actual speedup depends on the length of
        > the byte array fed into CRC32C and, to some extent, on the H/W
        > generation the test runs on.
        >
        >
        >
        > Thanks,
        >
        > Lutz
        >
        >
        >