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

[Doerr, Martin]

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