RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

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

RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

Schmidt, Lutz

Dear all,

 

I would like to request reviews for this s390-only enhancement:

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180823

Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180823.00/

 

The code for AES intrinsics in stubGenerator_s390.cpp is reworked to get rid of multiple copies of basically the same code snippets. Some chances for minor performance improvements are also taken advantage of.

 

The code has been active locally @SAP for several weeks now. No problems popped up.

 

Thank you!

Lutz

 

 

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

RE: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

Doerr, Martin

Hi Lutz,

 

nice change. Thanks for also fixing the stack pointer update order (missing part of 8180659). I didn’t find real bugs, but I’d like to get at least some comments fixed.

 

·         The comment in front of generate_push_Block mentions a “JIT_TIMER” timer which you didn’t contribute.

·         The comments describing the stack layout claim that there were requirements “min. size AES_parmBlk_addspace” and “Z_SP after expansion, octoword-aligned”. I think neither the minimal size nor the octoword alignment of the SP are required, so please remove these comments if this is correct. I believe that only the parmBlk needs the alignment.

·         The stack reservation is a little oversized. Maximum space used by alignment is AES_parmBlk_align-8. Please fix at least the comment. The spill space is also oversized (only 2 slots are used). I leave it up to you if you want to reduce stack space reservation. As this frame is only on top while processing AES stuff, it’s not really an issue to waste a few bytes.

·         generate_AES_cipherBlock contains an unnecessary rotate_then_insert instruction which seems to be a leftover from some removed functionality.

 

I can also sponsor this change after we go a 2nd review.

 

Best regards,

Martin

 

 

From: hotspot-compiler-dev [mailto:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Freitag, 28. Juli 2017 13:06
To: [hidden email]
Subject: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Dear all,

 

I would like to request reviews for this s390-only enhancement:

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180823

Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180823.00/

 

The code for AES intrinsics in stubGenerator_s390.cpp is reworked to get rid of multiple copies of basically the same code snippets. Some chances for minor performance improvements are also taken advantage of.

 

The code has been active locally @SAP for several weeks now. No problems popped up.

 

Thank you!

Lutz

 

 

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

Re: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

Schmidt, Lutz

Hi Martin,

 

Thanks for the review. Please find my comments re. your findings below, marked with “>>>”. The changes are reflected in a new webrev iteration at http://cr.openjdk.java.net/~lucy/webrevs/8180823.01/

 

Regards, Lutz

 

 

On 31.07.2017, 15:28, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

nice change. Thanks for also fixing the stack pointer update order (missing part of 8180659). I didn’t find real bugs, but I’d like to get at least some comments fixed.

 

  • The comment in front of generate_push_Block mentions a “JIT_TIMER” timer which you didn’t contribute.

>>>  You are right. JIT_TIMER is a SAP internal tool. Comment removed.

  • The comments describing the stack layout claim that there were requirements “min. size AES_parmBlk_addspace” and “Z_SP after expansion, octoword-aligned”. I think neither the minimal size nor the octoword alignment of the SP are required, so please remove these comments if this is correct. I believe that only the parmBlk needs the alignment.

>>>  Comments are fixed.

  • The stack reservation is a little oversized. Maximum space used by alignment is AES_parmBlk_align-8. Please fix at least the comment. The spill space is also oversized (only 2 slots are used). I leave it up to you if you want to reduce stack space reservation. As this frame is only on top while processing AES stuff, it’s not really an issue to waste a few bytes.

>>>  Add’l (spill) space requirement is adapted (3*8 bytes). I would like to keep the alignment stuff “as is”.

  • generate_AES_cipherBlock contains an unnecessary rotate_then_insert instruction which seems to be a leftover from some removed functionality.

>>>  removed. Was a JIT_TIMER leftover which wasn’t correctly annotated. 

 

I can also sponsor this change after we go a 2nd review.

 

Best regards,

Martin

 

 

From: hotspot-compiler-dev [mailto:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Freitag, 28. Juli 2017 13:06
To: [hidden email]
Subject: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Dear all,

 

I would like to request reviews for this s390-only enhancement:

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180823

Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180823.00/

 

The code for AES intrinsics in stubGenerator_s390.cpp is reworked to get rid of multiple copies of basically the same code snippets. Some chances for minor performance improvements are also taken advantage of.

 

The code has been active locally @SAP for several weeks now. No problems popped up.

 

Thank you!

Lutz

 

 

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

RE: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

Lindenmaier, Goetz

Hi Lutz,

 

I had a look at your change. It looks good. I ran the aes

jtreg tests on it which passed.

I’m not completely happy with the error message in vm_version,

as it differs from that on x86 with the same flag settings. I think

similar behavior of the platforms is desirable. But the string

seems not to be tested, thus does no harm to the tests.

 

Best regards,

  Goetz.

 

From: hotspot-compiler-dev [mailto:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Montag, 31. Juli 2017 17:43
To: Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Hi Martin,

 

Thanks for the review. Please find my comments re. your findings below, marked with “>>>”. The changes are reflected in a new webrev iteration at http://cr.openjdk.java.net/~lucy/webrevs/8180823.01/

 

Regards, Lutz

 

 

On 31.07.2017, 15:28, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

nice change. Thanks for also fixing the stack pointer update order (missing part of 8180659). I didn’t find real bugs, but I’d like to get at least some comments fixed.

 

  • The comment in front of generate_push_Block mentions a “JIT_TIMER” timer which you didn’t contribute.

>>>  You are right. JIT_TIMER is a SAP internal tool. Comment removed.

  • The comments describing the stack layout claim that there were requirements “min. size AES_parmBlk_addspace” and “Z_SP after expansion, octoword-aligned”. I think neither the minimal size nor the octoword alignment of the SP are required, so please remove these comments if this is correct. I believe that only the parmBlk needs the alignment.

>>>  Comments are fixed.

  • The stack reservation is a little oversized. Maximum space used by alignment is AES_parmBlk_align-8. Please fix at least the comment. The spill space is also oversized (only 2 slots are used). I leave it up to you if you want to reduce stack space reservation. As this frame is only on top while processing AES stuff, it’s not really an issue to waste a few bytes.

>>>  Add’l (spill) space requirement is adapted (3*8 bytes). I would like to keep the alignment stuff “as is”.

  • generate_AES_cipherBlock contains an unnecessary rotate_then_insert instruction which seems to be a leftover from some removed functionality.

>>>  removed. Was a JIT_TIMER leftover which wasn’t correctly annotated. 

 

I can also sponsor this change after we go a 2nd review.

 

Best regards,

Martin

 

 

From: hotspot-compiler-dev [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Freitag, 28. Juli 2017 13:06
To: [hidden email]
Subject: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Dear all,

 

I would like to request reviews for this s390-only enhancement:

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180823

Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180823.00/

 

The code for AES intrinsics in stubGenerator_s390.cpp is reworked to get rid of multiple copies of basically the same code snippets. Some chances for minor performance improvements are also taken advantage of.

 

The code has been active locally @SAP for several weeks now. No problems popped up.

 

Thank you!

Lutz

 

 

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

Re: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

Schmidt, Lutz

Thanks, Goetz, for having a look.

 

Regards, Lutz

 

On 01.08.2017, 14:58, "Lindenmaier, Goetz" <[hidden email]> wrote:

 

Hi Lutz,

 

I had a look at your change. It looks good. I ran the aes

jtreg tests on it which passed.

I’m not completely happy with the error message in vm_version,

as it differs from that on x86 with the same flag settings. I think

similar behavior of the platforms is desirable. But the string

seems not to be tested, thus does no harm to the tests.

 

Best regards,

  Goetz.

 

From: hotspot-compiler-dev [mailto:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Montag, 31. Juli 2017 17:43
To: Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Hi Martin,

 

Thanks for the review. Please find my comments re. your findings below, marked with “>>>”. The changes are reflected in a new webrev iteration at http://cr.openjdk.java.net/~lucy/webrevs/8180823.01/

 

Regards, Lutz

 

 

On 31.07.2017, 15:28, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

nice change. Thanks for also fixing the stack pointer update order (missing part of 8180659). I didn’t find real bugs, but I’d like to get at least some comments fixed.

 

  • The comment in front of generate_push_Block mentions a “JIT_TIMER” timer which you didn’t contribute.

>>>  You are right. JIT_TIMER is a SAP internal tool. Comment removed.

  • The comments describing the stack layout claim that there were requirements “min. size AES_parmBlk_addspace” and “Z_SP after expansion, octoword-aligned”. I think neither the minimal size nor the octoword alignment of the SP are required, so please remove these comments if this is correct. I believe that only the parmBlk needs the alignment.

>>>  Comments are fixed.

  • The stack reservation is a little oversized. Maximum space used by alignment is AES_parmBlk_align-8. Please fix at least the comment. The spill space is also oversized (only 2 slots are used). I leave it up to you if you want to reduce stack space reservation. As this frame is only on top while processing AES stuff, it’s not really an issue to waste a few bytes.

>>>  Add’l (spill) space requirement is adapted (3*8 bytes). I would like to keep the alignment stuff “as is”.

  • generate_AES_cipherBlock contains an unnecessary rotate_then_insert instruction which seems to be a leftover from some removed functionality.

>>>  removed. Was a JIT_TIMER leftover which wasn’t correctly annotated. 

 

I can also sponsor this change after we go a 2nd review.

 

Best regards,

Martin

 

 

From: hotspot-compiler-dev [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Freitag, 28. Juli 2017 13:06
To: [hidden email]
Subject: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Dear all,

 

I would like to request reviews for this s390-only enhancement:

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180823

Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180823.00/

 

The code for AES intrinsics in stubGenerator_s390.cpp is reworked to get rid of multiple copies of basically the same code snippets. Some chances for minor performance improvements are also taken advantage of.

 

The code has been active locally @SAP for several weeks now. No problems popped up.

 

Thank you!

Lutz

 

 

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

RE: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

Doerr, Martin

Pushed with adapted error message and updated copyright.

 

Best regards,

Martin

 

From: Schmidt, Lutz
Sent: Dienstag, 1. August 2017 15:06
To: Lindenmaier, Goetz <[hidden email]>; Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Thanks, Goetz, for having a look.

 

Regards, Lutz

 

On 01.08.2017, 14:58, "Lindenmaier, Goetz" <[hidden email]> wrote:

 

Hi Lutz,

 

I had a look at your change. It looks good. I ran the aes

jtreg tests on it which passed.

I’m not completely happy with the error message in vm_version,

as it differs from that on x86 with the same flag settings. I think

similar behavior of the platforms is desirable. But the string

seems not to be tested, thus does no harm to the tests.

 

Best regards,

  Goetz.

 

From: hotspot-compiler-dev [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Montag, 31. Juli 2017 17:43
To: Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Hi Martin,

 

Thanks for the review. Please find my comments re. your findings below, marked with “>>>”. The changes are reflected in a new webrev iteration at http://cr.openjdk.java.net/~lucy/webrevs/8180823.01/

 

Regards, Lutz

 

 

On 31.07.2017, 15:28, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

nice change. Thanks for also fixing the stack pointer update order (missing part of 8180659). I didn’t find real bugs, but I’d like to get at least some comments fixed.

 

  • The comment in front of generate_push_Block mentions a “JIT_TIMER” timer which you didn’t contribute.

>>>  You are right. JIT_TIMER is a SAP internal tool. Comment removed.

  • The comments describing the stack layout claim that there were requirements “min. size AES_parmBlk_addspace” and “Z_SP after expansion, octoword-aligned”. I think neither the minimal size nor the octoword alignment of the SP are required, so please remove these comments if this is correct. I believe that only the parmBlk needs the alignment.

>>>  Comments are fixed.

  • The stack reservation is a little oversized. Maximum space used by alignment is AES_parmBlk_align-8. Please fix at least the comment. The spill space is also oversized (only 2 slots are used). I leave it up to you if you want to reduce stack space reservation. As this frame is only on top while processing AES stuff, it’s not really an issue to waste a few bytes.

>>>  Add’l (spill) space requirement is adapted (3*8 bytes). I would like to keep the alignment stuff “as is”.

  • generate_AES_cipherBlock contains an unnecessary rotate_then_insert instruction which seems to be a leftover from some removed functionality.

>>>  removed. Was a JIT_TIMER leftover which wasn’t correctly annotated. 

 

I can also sponsor this change after we go a 2nd review.

 

Best regards,

Martin

 

 

From: hotspot-compiler-dev [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Freitag, 28. Juli 2017 13:06
To: [hidden email]
Subject: RFR(M) 8180823: [s390] Rework/optimize AES intrinsics

 

Dear all,

 

I would like to request reviews for this s390-only enhancement:

Bug:    https://bugs.openjdk.java.net/browse/JDK-8180823

Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180823.00/

 

The code for AES intrinsics in stubGenerator_s390.cpp is reworked to get rid of multiple copies of basically the same code snippets. Some chances for minor performance improvements are also taken advantage of.

 

The code has been active locally @SAP for several weeks now. No problems popped up.

 

Thank you!

Lutz

 

 

Loading...