RFR (M) 8175369: [ppc] Provide intrinsic implementation for CRC32C
Schmidt, Lutz
lutz.schmidt at sap.com
Thu Mar 9 08:41:47 UTC 2017
Thank you, Vladimir!
Regards, Lutz
On 09.03.17, 01:39, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
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
> >
> >
>
>
More information about the hotspot-compiler-dev
mailing list