RFR (M) 8175369: [ppc] Provide intrinsic implementation for CRC32C

[Volker Simonis]

Hi Lutz, Vladimir,

change looks good!

Your webrev for the s390 version of the CRC32C intrinsics
(http://cr.openjdk.java.net/~lucy/webrevs/8175368.03/) doesn't display
any shared changes, and that's actually what you apparently wanted to
change in the last version (i.e. move the changes to c1_Compiler.cpp
into this change). Unfortunately, the patch file from that webrev
still contains these shared change.

I'm not sure how that could happen? It may be a problem with
webrev.ksh in combination with the Mercurial Queues extension and the
fact that you had a local change in the repository from which you've
created the webrev.

So to cut a long story short, I've just pushed Lutz's s390 change
(http://hg.openjdk.java.net/jdk10/hs/hotspot/rev/18ace35ee108) in good
faith that it only contains s390 specific changes and unintentionally
updated c1_Compiler.cpp. This requires yet another version of this
change because it also updates c1_Compiler.cpp:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8175369/


@Vladimir (or anybody else from Oracle) - could you please sponsor
this reviewed (and besides the tiny change in c1_Compiler.cpp, ppc64
only) change?

Thank you and sorry for the confusion,
Volker


On Wed, Mar 8, 2017 at 3:38 PM, Schmidt, Lutz <lutz.schmidt at sap.com> wrote:
> Martin, thanks for reviewing my change!
>
>
>
> All,
>
> Sorry for the delayed response. I had to take care of some urgent private
> stuff the last few days.
>
>
>
> After rebasing, I have updated the comment in kernel_crc32_1word. It was
> outdated anyway. I would like to postpone the decision about the fate of
> kernel_crc32_2word.
>
>
>
> The conflict in share/vm/c1/c1_compiler.cpp is somewhat tricky. I decided to
> remove the file from my (8175368, s390) webrev and to enable both cpus (ppc
> and s390) with this change. This “trick” avoids conflicting changes to the
> same source code line.
>
>
>
> Please find the most recent webrev here:
> http://cr.openjdk.java.net/~lucy/webrevs/8175369.01/
>
>
>
> Thank you,
>
> Lutz
>
>
>
>
>
> On 03.03.17, 17:33, "Doerr, Martin" <martin.doerr at sap.com> wrote:
>
>
>
> Hi Lutz,
>
>
>
> thank you very much for implementing it for PPC64, too.
>
>
>
> Unfortunately, the patch doesn’t apply cleanly any more (due to clashes with
> other changes).
>
> But it should be easy to fix.
>
> The copyright change of templateInterpreterGenerator_ppc.cpp doesn’t fit any
> more (same is true for s390 webrev).
>
> In addition, the PPC64 change was not build on top of the s390 one. The
> shared code change in c1_Compiler.cpp needs to be adapted in this change and
> pushed after the s390 one.
>
>
>
> As the change is very similar to what you’ve done on s390, it already
> contains what I had requested in the other review.
>
> I only have a comment on the macroAssembler part:
>
> I didn't review kernel_crc32_2word as it is no longer used and should get
> removed sooner or later (not necessarily with this change).
>
> kernel_crc32_1word contains an s390 comment "ahgi".
>
>
>
> Besides that, it looks good to me.
>
>
>
> Thanks and best regards,
>
> Martin
>
>
>
>
>
> From: hotspot-compiler-dev
> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Schmidt,
> Lutz
> Sent: Donnerstag, 2. März 2017 17:24
> To: hotspot-compiler-dev at openjdk.java.net
> Subject: RFR (M) 8175369: [ppc] Provide intrinsic implementation for CRC32C
>
>
>
> Hi all,
>
>
>
> may I kindly request reviews for my medium size enhancement? Further down
> the road, I would need a sponsor, too.
>
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175369
>
> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8175369/
>
>
>
> Description:
>
> This intrinsic implementation provides some performance benefit over the
> standard Java implementation. It uses only well-known “standard”
> instructions, available on all supported POWER cpus. Being very similar to
> the CRC32 intrinsics, it was tried to share as much code as possible between
> these two.
>
>
>
> Performance was observed to increase up to 2.0x, depending on the length of
> the byte array fed into CRC32C. Short byte arrays benefit more. That is due
> to the fact that the Java implementation of CRC32C is well inlined and
> optimized by the JIT, at least with my simple micro benchmarks.
>
>
>
> Thanks,
>
> Lutz
>
>