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

[Schmidt, Lutz]

Hi all,

I have fixed a tiny little bug (copy result to result register) in c1_LIRAssembler_s390.cpp and created a new webrev:
  http://cr.openjdk.java.net/~lucy/webrevs/8175368.02/

I believe I still need one more review. Furthermore: any volunteer to sponsor my change?

Thanks,
Lutz



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

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