Quantcast

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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Schmidt, Lutz

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

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Doerr, Martin

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:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Donnerstag, 2. März 2017 17:24
To: [hidden email]
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

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Schmidt, Lutz

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" <[hidden email]> 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:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Donnerstag, 2. März 2017 17:24

To: [hidden email]
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

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]> 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" <[hidden email]> 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:[hidden email]] On Behalf Of Schmidt,
> Lutz
> Sent: Donnerstag, 2. März 2017 17:24
> To: [hidden email]
> 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
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Schmidt, Lutz
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" <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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:[hidden email]] On Behalf Of Schmidt,
    > Lutz
    > Sent: Donnerstag, 2. März 2017 17:24
    > To: [hidden email]
    > 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
    >
    >
   

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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" <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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:[hidden email]] On Behalf Of Schmidt,
>     > Lutz
>     > Sent: Donnerstag, 2. März 2017 17:24
>     > To: [hidden email]
>     > 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
>     >
>     >
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Schmidt, Lutz
Thank you, Vladimir!
Regards, Lutz



On 09.03.17, 01:39, "Vladimir Kozlov" <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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:[hidden email]] On Behalf Of Schmidt,
    >     > Lutz
    >     > Sent: Donnerstag, 2. März 2017 17:24
    >     > To: [hidden email]
    >     > 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
    >     >
    >     >
    >
    >
   

Loading...