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

[Vladimir Kozlov]

I read 8175368 thread first so I thought we need additional bug to remove shared changes.
But since this 8175369 changes have update for c1_Compiler.cpp we can use it to sort everything out.

Changes to c1_Compiler.cpp are very simple so they are fine to me. I will sponsor and resolve conflict.

Thanks,
Vladimir

On 3/8/17 8:30 AM, Schmidt, Lutz wrote:
> Volker, Vladimir,
>
> I guess I have to apologize for this mess! Obviously, I haven’t mastered the tools yet. I will try hard to prevent this from happening again.
>
> Besides that, thank you Volker for reviewing my ppc change and for fixing the issue with the s390 change.
>
> Regards,
> Lutz
>
>
>
> On 08.03.17, 17:20, "Volker Simonis" <volker.simonis at gmail.com> wrote:
>
>     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
>     >
>     >
>
>