Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

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

Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Schmidt, Lutz

Hi Martin et al,

 

Finally, here is the expanded rework of my change. Doesn’t look big, but it took some time to make sure I didn’t miss places that need adaptation.

 

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

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

 

Please have a look.

 

Thanks, Lutz

 

Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834

 

 

On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:

 

Hi Martin,

 

thank you for your comments! They triggered some deeper thoughts.

 

I could not find any written proof that it is ok to write outside the bounds given by the SP. The risk is minimal, though. You would need an external interrupt to hit the gap between the write and the SP update.

 

There is more than one code location affected. Doing it right will definitely blow up the fix size. There is not enough time right now: I will leave the office for vacation in an hour or so. I will resume working on the issue once I’m back (June 19th).

 

Best regards,

Lutz

 

On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

thanks for providing the webrev.

 

Is it allowed to write to the stack before updating the SP?

I know that the PPC ABI allows this within a certain range, but I’m not aware of such an exception on s390x.

 

I’d also prefer separate functions instead of one with so many cases.

E.g. one function which copies the fp and one which takes a given fp like:

void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {

  assert_different_registers(newSP, fp, Z_SP);

  z_lgr(Z_SP, newSP);

  z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);

}

 

Thanks and best regards,

Martin

 

 

From: hotspot-compiler-dev [mailto:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 23. Mai 2017 12:47
To: [hidden email]
Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()

 

Dear all,

 

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

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

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

 

Thank you!

Lutz

 

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

RE: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Doerr, Martin

Hi Lutz,

 

nice change. Thanks for fixing writes below SP and also for improving readability.

 

I think the new pop_frame function should better be called e.g. pop_frame_and_restore_retPC because it performs this additional step in contrast to the other pop_frame function.

 

s390.ad needs a copyright update.

 

Besides that, it looks good to me. I can sponsor this change.

 

Best regards,

Martin

 

 

 

From: Schmidt, Lutz
Sent: Donnerstag, 29. Juni 2017 10:48
To: Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

 

Hi Martin et al,

 

Finally, here is the expanded rework of my change. Doesn’t look big, but it took some time to make sure I didn’t miss places that need adaptation.

 

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

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

 

Please have a look.

 

Thanks, Lutz

 

Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834

 

 

On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:

 

Hi Martin,

 

thank you for your comments! They triggered some deeper thoughts.

 

I could not find any written proof that it is ok to write outside the bounds given by the SP. The risk is minimal, though. You would need an external interrupt to hit the gap between the write and the SP update.

 

There is more than one code location affected. Doing it right will definitely blow up the fix size. There is not enough time right now: I will leave the office for vacation in an hour or so. I will resume working on the issue once I’m back (June 19th).

 

Best regards,

Lutz

 

On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

thanks for providing the webrev.

 

Is it allowed to write to the stack before updating the SP?

I know that the PPC ABI allows this within a certain range, but I’m not aware of such an exception on s390x.

 

I’d also prefer separate functions instead of one with so many cases.

E.g. one function which copies the fp and one which takes a given fp like:

void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {

  assert_different_registers(newSP, fp, Z_SP);

  z_lgr(Z_SP, newSP);

  z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);

}

 

Thanks and best regards,

Martin

 

 

From: hotspot-compiler-dev [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 23. Mai 2017 12:47
To:
[hidden email]
Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()

 

Dear all,

 

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

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

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

 

Thank you!

Lutz

 

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

Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Schmidt, Lutz

Thanks, Martin!

 

I have renamed that one function and the copyright was updated as well. The webrev was updated in-place.

 

Regards, Lutz

 

On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

nice change. Thanks for fixing writes below SP and also for improving readability.

 

I think the new pop_frame function should better be called e.g. pop_frame_and_restore_retPC because it performs this additional step in contrast to the other pop_frame function.

 

s390.ad needs a copyright update.

 

Besides that, it looks good to me. I can sponsor this change.

 

Best regards,

Martin

 

 

 

From: Schmidt, Lutz
Sent: Donnerstag, 29. Juni 2017 10:48
To: Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

 

Hi Martin et al,

 

Finally, here is the expanded rework of my change. Doesn’t look big, but it took some time to make sure I didn’t miss places that need adaptation.

 

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

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

 

Please have a look.

 

Thanks, Lutz

 

Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834

 

 

On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:

 

Hi Martin,

 

thank you for your comments! They triggered some deeper thoughts.

 

I could not find any written proof that it is ok to write outside the bounds given by the SP. The risk is minimal, though. You would need an external interrupt to hit the gap between the write and the SP update.

 

There is more than one code location affected. Doing it right will definitely blow up the fix size. There is not enough time right now: I will leave the office for vacation in an hour or so. I will resume working on the issue once I’m back (June 19th).

 

Best regards,

Lutz

 

On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:

 

Hi Lutz,

 

thanks for providing the webrev.

 

Is it allowed to write to the stack before updating the SP?

I know that the PPC ABI allows this within a certain range, but I’m not aware of such an exception on s390x.

 

I’d also prefer separate functions instead of one with so many cases.

E.g. one function which copies the fp and one which takes a given fp like:

void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {

  assert_different_registers(newSP, fp, Z_SP);

  z_lgr(Z_SP, newSP);

  z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);

}

 

Thanks and best regards,

Martin

 

 

From: hotspot-compiler-dev [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 23. Mai 2017 12:47
To:
[hidden email]
Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()

 

Dear all,

 

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

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

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

 

Thank you!

Lutz

 

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

Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Volker Simonis
Hi Lutz,

the change looks good - I just have tow small questions/comments:

2050   assert_different_registers(newSP, Z_R0); // required to
generate efficient code.
...
2056   z_lgr(Z_SP, newSP);
2057   if (newSP != Z_R0) { // despite the assert above, make sure we
generate correct code.

I think either we are sure that we don't call resize_frame_absolute()
with 'newSP == Z_R0' (I think we currently can be as I've checked all
the call sites) and keep just the assertion in order to check against
future violations of this assumption. Or we think there will be
future, valid uses of resize_frame_absolute() with 'newSP == Z_R0' in
which case we should remove the assertion and keep the alternate code
generation path.

Also, as far as I understand, this change should fix writes below the
SP, but there are still other places (notably in
generate_push_parmBlk()) where we write below the SP before calling
resize_frame_absolute() to adjust it. Will you fix them in a follow-up
change?

Thanks for fixing this,
Volker



On Thu, Jun 29, 2017 at 12:14 PM, Schmidt, Lutz <[hidden email]> wrote:

> Thanks, Martin!
>
>
>
> I have renamed that one function and the copyright was updated as well. The
> webrev was updated in-place.
>
>
>
> Regards, Lutz
>
>
>
> On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:
>
>
>
> Hi Lutz,
>
>
>
> nice change. Thanks for fixing writes below SP and also for improving
> readability.
>
>
>
> I think the new pop_frame function should better be called e.g.
> pop_frame_and_restore_retPC because it performs this additional step in
> contrast to the other pop_frame function.
>
>
>
> s390.ad needs a copyright update.
>
>
>
> Besides that, it looks good to me. I can sponsor this change.
>
>
>
> Best regards,
>
> Martin
>
>
>
>
>
>
>
> From: Schmidt, Lutz
> Sent: Donnerstag, 29. Juni 2017 10:48
> To: Doerr, Martin <[hidden email]>;
> [hidden email]
> Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in
> resize_frame_absolute()
>
>
>
> Hi Martin et al,
>
>
>
> Finally, here is the expanded rework of my change. Doesn’t look big, but it
> took some time to make sure I didn’t miss places that need adaptation.
>
>
>
> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
>
> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.01/
>
>
>
> Please have a look.
>
>
>
> Thanks, Lutz
>
>
>
> Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
>
>
>
>
>
> On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:
>
>
>
> Hi Martin,
>
>
>
> thank you for your comments! They triggered some deeper thoughts.
>
>
>
> I could not find any written proof that it is ok to write outside the bounds
> given by the SP. The risk is minimal, though. You would need an external
> interrupt to hit the gap between the write and the SP update.
>
>
>
> There is more than one code location affected. Doing it right will
> definitely blow up the fix size. There is not enough time right now: I will
> leave the office for vacation in an hour or so. I will resume working on the
> issue once I’m back (June 19th).
>
>
>
> Best regards,
>
> Lutz
>
>
>
> On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:
>
>
>
> Hi Lutz,
>
>
>
> thanks for providing the webrev.
>
>
>
> Is it allowed to write to the stack before updating the SP?
>
> I know that the PPC ABI allows this within a certain range, but I’m not
> aware of such an exception on s390x.
>
>
>
> I’d also prefer separate functions instead of one with so many cases.
>
> E.g. one function which copies the fp and one which takes a given fp like:
>
> void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {
>
>   assert_different_registers(newSP, fp, Z_SP);
>
>   z_lgr(Z_SP, newSP);
>
>   z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);
>
> }
>
>
>
> Thanks and best regards,
>
> Martin
>
>
>
>
>
> From: hotspot-compiler-dev
> [mailto:[hidden email]] On Behalf Of Schmidt,
> Lutz
> Sent: Dienstag, 23. Mai 2017 12:47
> To: [hidden email]
> Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()
>
>
>
> Dear all,
>
>
>
> I would like to request reviews for this tiny, s390-only enhancement:
>
> Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
>
> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.00/
>
>
>
> Thank you!
>
> Lutz
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Schmidt, Lutz
Thank you, Volker, for your in-depth review.

As we can never be sure what the future brings, and because we are always short on registers on s390, I opt for “take the assertion out”. Done.

Well observed! There is one place left (in generate_push_parmBlk()) where we access the stack outside the bounds given by Z_SP. I left the code “as is” for now. With the fix for JDK-8180823 (ready for review as soon as this one is through), the out-of-bounds access will go away.

While re-testing, I ran into crashes which were obviously related to my changes and which I had not seen before. Analyzing the crashes and finding/fixing the root cause took quite a while: push_frame() now has to use a scratch register, and two callers relied on that scratch register to remain unaltered.

The issue is solved, all tests we run inhouse pass and I have created a new webrev:
http://cr.openjdk.java.net/~lucy/webrevs/8180659.02/ 

Please have a (hopefully final) look. Thank you!

Regards,
Lutz

 

On 30.06.2017, 11:37, "Volker Simonis" <[hidden email]> wrote:

    Hi Lutz,
   
    the change looks good - I just have tow small questions/comments:
   
    2050   assert_different_registers(newSP, Z_R0); // required to
    generate efficient code.
    ...
    2056   z_lgr(Z_SP, newSP);
    2057   if (newSP != Z_R0) { // despite the assert above, make sure we
    generate correct code.
   
    I think either we are sure that we don't call resize_frame_absolute()
    with 'newSP == Z_R0' (I think we currently can be as I've checked all
    the call sites) and keep just the assertion in order to check against
    future violations of this assumption. Or we think there will be
    future, valid uses of resize_frame_absolute() with 'newSP == Z_R0' in
    which case we should remove the assertion and keep the alternate code
    generation path.
   
    Also, as far as I understand, this change should fix writes below the
    SP, but there are still other places (notably in
    generate_push_parmBlk()) where we write below the SP before calling
    resize_frame_absolute() to adjust it. Will you fix them in a follow-up
    change?
   
    Thanks for fixing this,
    Volker
   
   
   
    On Thu, Jun 29, 2017 at 12:14 PM, Schmidt, Lutz <[hidden email]> wrote:
    > Thanks, Martin!
    >
    >
    >
    > I have renamed that one function and the copyright was updated as well. The
    > webrev was updated in-place.
    >
    >
    >
    > Regards, Lutz
    >
    >
    >
    > On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:
    >
    >
    >
    > Hi Lutz,
    >
    >
    >
    > nice change. Thanks for fixing writes below SP and also for improving
    > readability.
    >
    >
    >
    > I think the new pop_frame function should better be called e.g.
    > pop_frame_and_restore_retPC because it performs this additional step in
    > contrast to the other pop_frame function.
    >
    >
    >
    > s390.ad needs a copyright update.
    >
    >
    >
    > Besides that, it looks good to me. I can sponsor this change.
    >
    >
    >
    > Best regards,
    >
    > Martin
    >
    >
    >
    >
    >
    >
    >
    > From: Schmidt, Lutz
    > Sent: Donnerstag, 29. Juni 2017 10:48
    > To: Doerr, Martin <[hidden email]>;
    > [hidden email]
    > Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in
    > resize_frame_absolute()
    >
    >
    >
    > Hi Martin et al,
    >
    >
    >
    > Finally, here is the expanded rework of my change. Doesn’t look big, but it
    > took some time to make sure I didn’t miss places that need adaptation.
    >
    >
    >
    > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
    >
    > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.01/
    >
    >
    >
    > Please have a look.
    >
    >
    >
    > Thanks, Lutz
    >
    >
    >
    > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
    >
    >
    >
    >
    >
    > On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:
    >
    >
    >
    > Hi Martin,
    >
    >
    >
    > thank you for your comments! They triggered some deeper thoughts.
    >
    >
    >
    > I could not find any written proof that it is ok to write outside the bounds
    > given by the SP. The risk is minimal, though. You would need an external
    > interrupt to hit the gap between the write and the SP update.
    >
    >
    >
    > There is more than one code location affected. Doing it right will
    > definitely blow up the fix size. There is not enough time right now: I will
    > leave the office for vacation in an hour or so. I will resume working on the
    > issue once I’m back (June 19th).
    >
    >
    >
    > Best regards,
    >
    > Lutz
    >
    >
    >
    > On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:
    >
    >
    >
    > Hi Lutz,
    >
    >
    >
    > thanks for providing the webrev.
    >
    >
    >
    > Is it allowed to write to the stack before updating the SP?
    >
    > I know that the PPC ABI allows this within a certain range, but I’m not
    > aware of such an exception on s390x.
    >
    >
    >
    > I’d also prefer separate functions instead of one with so many cases.
    >
    > E.g. one function which copies the fp and one which takes a given fp like:
    >
    > void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {
    >
    >   assert_different_registers(newSP, fp, Z_SP);
    >
    >   z_lgr(Z_SP, newSP);
    >
    >   z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);
    >
    > }
    >
    >
    >
    > Thanks and best regards,
    >
    > Martin
    >
    >
    >
    >
    >
    > From: hotspot-compiler-dev
    > [mailto:[hidden email]] On Behalf Of Schmidt,
    > Lutz
    > Sent: Dienstag, 23. Mai 2017 12:47
    > To: [hidden email]
    > Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()
    >
    >
    >
    > Dear all,
    >
    >
    >
    > I would like to request reviews for this tiny, s390-only enhancement:
    >
    > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
    >
    > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.00/
    >
    >
    >
    > Thank you!
    >
    > Lutz
    >
    >
   





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

RE: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Doerr, Martin
Hi Lutz,

Thanks for doing all the work. I have a couple of small change requests:

macroAssembler_s390.cpp:
push_frame:
- z_stg(old_sp,... can be moved out of the if ... else ...

g1_write_barrier_pre:
- Wouldn't assert or guarantee be better than z_SIGSEGV?
- lgr_if_needed(Rpre_val, Rpre_save); can be replaced by using Rpre_save in the succeeding call_VM_leaf.

verify_oop:
- I think nbytes_save should be 5*BytesPerWord because we're saving 5 regs. The "+ BytesPerWord" should not be needed. (Stack alignment on s390 is only 8 Bytes if I remember correctly.)


stubGenerator_s390.cpp:
- Some comments mix up "implicitly done in save_live_registers" and "implicitly done in restore_live_registers".
- When fixing this, you could also replace the double semicolons.

The rest looks good. Also, thanks for doing some cleanup.

I'm ok with handling generate_push_parmBlk() in the separate bug.

Btw. please keep the "[s390]" on the right side of the bug id and ":" in the email headline for future RFRs. Otherwise, it gets easily lost in hg log when pushing.

Best regards,
Martin


-----Original Message-----
From: Schmidt, Lutz
Sent: Mittwoch, 12. Juli 2017 10:36
To: Volker Simonis <[hidden email]>
Cc: Doerr, Martin <[hidden email]>; [hidden email]
Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Thank you, Volker, for your in-depth review.

As we can never be sure what the future brings, and because we are always short on registers on s390, I opt for “take the assertion out”. Done.

Well observed! There is one place left (in generate_push_parmBlk()) where we access the stack outside the bounds given by Z_SP. I left the code “as is” for now. With the fix for JDK-8180823 (ready for review as soon as this one is through), the out-of-bounds access will go away.

While re-testing, I ran into crashes which were obviously related to my changes and which I had not seen before. Analyzing the crashes and finding/fixing the root cause took quite a while: push_frame() now has to use a scratch register, and two callers relied on that scratch register to remain unaltered.

The issue is solved, all tests we run inhouse pass and I have created a new webrev:
http://cr.openjdk.java.net/~lucy/webrevs/8180659.02/ 

Please have a (hopefully final) look. Thank you!

Regards,
Lutz

 

On 30.06.2017, 11:37, "Volker Simonis" <[hidden email]> wrote:

    Hi Lutz,
   
    the change looks good - I just have tow small questions/comments:
   
    2050   assert_different_registers(newSP, Z_R0); // required to
    generate efficient code.
    ...
    2056   z_lgr(Z_SP, newSP);
    2057   if (newSP != Z_R0) { // despite the assert above, make sure we
    generate correct code.
   
    I think either we are sure that we don't call resize_frame_absolute()
    with 'newSP == Z_R0' (I think we currently can be as I've checked all
    the call sites) and keep just the assertion in order to check against
    future violations of this assumption. Or we think there will be
    future, valid uses of resize_frame_absolute() with 'newSP == Z_R0' in
    which case we should remove the assertion and keep the alternate code
    generation path.
   
    Also, as far as I understand, this change should fix writes below the
    SP, but there are still other places (notably in
    generate_push_parmBlk()) where we write below the SP before calling
    resize_frame_absolute() to adjust it. Will you fix them in a follow-up
    change?
   
    Thanks for fixing this,
    Volker
   
   
   
    On Thu, Jun 29, 2017 at 12:14 PM, Schmidt, Lutz <[hidden email]> wrote:
    > Thanks, Martin!
    >
    >
    >
    > I have renamed that one function and the copyright was updated as well. The
    > webrev was updated in-place.
    >
    >
    >
    > Regards, Lutz
    >
    >
    >
    > On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:
    >
    >
    >
    > Hi Lutz,
    >
    >
    >
    > nice change. Thanks for fixing writes below SP and also for improving
    > readability.
    >
    >
    >
    > I think the new pop_frame function should better be called e.g.
    > pop_frame_and_restore_retPC because it performs this additional step in
    > contrast to the other pop_frame function.
    >
    >
    >
    > s390.ad needs a copyright update.
    >
    >
    >
    > Besides that, it looks good to me. I can sponsor this change.
    >
    >
    >
    > Best regards,
    >
    > Martin
    >
    >
    >
    >
    >
    >
    >
    > From: Schmidt, Lutz
    > Sent: Donnerstag, 29. Juni 2017 10:48
    > To: Doerr, Martin <[hidden email]>;
    > [hidden email]
    > Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in
    > resize_frame_absolute()
    >
    >
    >
    > Hi Martin et al,
    >
    >
    >
    > Finally, here is the expanded rework of my change. Doesn’t look big, but it
    > took some time to make sure I didn’t miss places that need adaptation.
    >
    >
    >
    > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
    >
    > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.01/
    >
    >
    >
    > Please have a look.
    >
    >
    >
    > Thanks, Lutz
    >
    >
    >
    > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
    >
    >
    >
    >
    >
    > On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:
    >
    >
    >
    > Hi Martin,
    >
    >
    >
    > thank you for your comments! They triggered some deeper thoughts.
    >
    >
    >
    > I could not find any written proof that it is ok to write outside the bounds
    > given by the SP. The risk is minimal, though. You would need an external
    > interrupt to hit the gap between the write and the SP update.
    >
    >
    >
    > There is more than one code location affected. Doing it right will
    > definitely blow up the fix size. There is not enough time right now: I will
    > leave the office for vacation in an hour or so. I will resume working on the
    > issue once I’m back (June 19th).
    >
    >
    >
    > Best regards,
    >
    > Lutz
    >
    >
    >
    > On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:
    >
    >
    >
    > Hi Lutz,
    >
    >
    >
    > thanks for providing the webrev.
    >
    >
    >
    > Is it allowed to write to the stack before updating the SP?
    >
    > I know that the PPC ABI allows this within a certain range, but I’m not
    > aware of such an exception on s390x.
    >
    >
    >
    > I’d also prefer separate functions instead of one with so many cases.
    >
    > E.g. one function which copies the fp and one which takes a given fp like:
    >
    > void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {
    >
    >   assert_different_registers(newSP, fp, Z_SP);
    >
    >   z_lgr(Z_SP, newSP);
    >
    >   z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);
    >
    > }
    >
    >
    >
    > Thanks and best regards,
    >
    > Martin
    >
    >
    >
    >
    >
    > From: hotspot-compiler-dev
    > [mailto:[hidden email]] On Behalf Of Schmidt,
    > Lutz
    > Sent: Dienstag, 23. Mai 2017 12:47
    > To: [hidden email]
    > Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()
    >
    >
    >
    > Dear all,
    >
    >
    >
    > I would like to request reviews for this tiny, s390-only enhancement:
    >
    > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
    >
    > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.00/
    >
    >
    >
    > Thank you!
    >
    > Lutz
    >
    >
   





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

Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Schmidt, Lutz
Hi Martin,

thank you for reviewing my change. Please see my “>>>” inline comments below.

Please find the new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8180659.03/

Regards,
Lutz

 
Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834

 

On 25.07.2017, 19:25, "Doerr, Martin" <[hidden email]> wrote:

    Hi Lutz,
   
    Thanks for doing all the work. I have a couple of small change requests:
   
    macroAssembler_s390.cpp:
    push_frame:
    - z_stg(old_sp,... can be moved out of the if ... else ...
>>> done.
   
    g1_write_barrier_pre:
    - Wouldn't assert or guarantee be better than z_SIGSEGV?
>>> agree & changed. Z_SIGSEGV() was a leftover.
    - lgr_if_needed(Rpre_val, Rpre_save); can be replaced by using Rpre_save in the succeeding call_VM_leaf.
>>> do not agree (see comment on Rpre_val above g1_write_barrier_pre).
   
    verify_oop:
    - I think nbytes_save should be 5*BytesPerWord because we're saving 5 regs. The "+ BytesPerWord" should not be needed. (Stack alignment on s390 is only 8 Bytes if I remember correctly.)
>>> intention was to change stack layout as little as possible. The previous code saved Z_R0 as well – which is nonsense now. But OK. Modified to use only five slots.
   
   
    stubGenerator_s390.cpp:
    - Some comments mix up "implicitly done in save_live_registers" and "implicitly done in restore_live_registers".
    - When fixing this, you could also replace the double semicolons.
>>> good catch! Comments adapted. Changed ;; -> ;.
   
    The rest looks good. Also, thanks for doing some cleanup.
   
    I'm ok with handling generate_push_parmBlk() in the separate bug.
   
    Btw. please keep the "[s390]" on the right side of the bug id and ":" in the email headline for future RFRs. Otherwise, it gets easily lost in hg log when pushing.
   
    Best regards,
    Martin
   
   
    -----Original Message-----
    From: Schmidt, Lutz
    Sent: Mittwoch, 12. Juli 2017 10:36
    To: Volker Simonis <[hidden email]>
    Cc: Doerr, Martin <[hidden email]>; [hidden email]
    Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()
   
    Thank you, Volker, for your in-depth review.
   
    As we can never be sure what the future brings, and because we are always short on registers on s390, I opt for “take the assertion out”. Done.
   
    Well observed! There is one place left (in generate_push_parmBlk()) where we access the stack outside the bounds given by Z_SP. I left the code “as is” for now. With the fix for JDK-8180823 (ready for review as soon as this one is through), the out-of-bounds access will go away.
   
    While re-testing, I ran into crashes which were obviously related to my changes and which I had not seen before. Analyzing the crashes and finding/fixing the root cause took quite a while: push_frame() now has to use a scratch register, and two callers relied on that scratch register to remain unaltered.
   
    The issue is solved, all tests we run inhouse pass and I have created a new webrev:
    http://cr.openjdk.java.net/~lucy/webrevs/8180659.02/ 
   
    Please have a (hopefully final) look. Thank you!
   
    Regards,
    Lutz
   
     
   
    On 30.06.2017, 11:37, "Volker Simonis" <[hidden email]> wrote:
   
        Hi Lutz,
       
        the change looks good - I just have tow small questions/comments:
       
        2050   assert_different_registers(newSP, Z_R0); // required to
        generate efficient code.
        ...
        2056   z_lgr(Z_SP, newSP);
        2057   if (newSP != Z_R0) { // despite the assert above, make sure we
        generate correct code.
       
        I think either we are sure that we don't call resize_frame_absolute()
        with 'newSP == Z_R0' (I think we currently can be as I've checked all
        the call sites) and keep just the assertion in order to check against
        future violations of this assumption. Or we think there will be
        future, valid uses of resize_frame_absolute() with 'newSP == Z_R0' in
        which case we should remove the assertion and keep the alternate code
        generation path.
       
        Also, as far as I understand, this change should fix writes below the
        SP, but there are still other places (notably in
        generate_push_parmBlk()) where we write below the SP before calling
        resize_frame_absolute() to adjust it. Will you fix them in a follow-up
        change?
       
        Thanks for fixing this,
        Volker
       
       
       
        On Thu, Jun 29, 2017 at 12:14 PM, Schmidt, Lutz <[hidden email]> wrote:
        > Thanks, Martin!
        >
        >
        >
        > I have renamed that one function and the copyright was updated as well. The
        > webrev was updated in-place.
        >
        >
        >
        > Regards, Lutz
        >
        >
        >
        > On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:
        >
        >
        >
        > Hi Lutz,
        >
        >
        >
        > nice change. Thanks for fixing writes below SP and also for improving
        > readability.
        >
        >
        >
        > I think the new pop_frame function should better be called e.g.
        > pop_frame_and_restore_retPC because it performs this additional step in
        > contrast to the other pop_frame function.
        >
        >
        >
        > s390.ad needs a copyright update.
        >
        >
        >
        > Besides that, it looks good to me. I can sponsor this change.
        >
        >
        >
        > Best regards,
        >
        > Martin
        >
        >
        >
        >
        >
        >
        >
        > From: Schmidt, Lutz
        > Sent: Donnerstag, 29. Juni 2017 10:48
        > To: Doerr, Martin <[hidden email]>;
        > [hidden email]
        > Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in
        > resize_frame_absolute()
        >
        >
        >
        > Hi Martin et al,
        >
        >
        >
        > Finally, here is the expanded rework of my change. Doesn’t look big, but it
        > took some time to make sure I didn’t miss places that need adaptation.
        >
        >
        >
        > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
        >
        > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.01/
        >
        >
        >
        > Please have a look.
        >
        >
        >
        > Thanks, Lutz
        >
        >
        >
        > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
        >
        >
        >
        >
        >
        > On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:
        >
        >
        >
        > Hi Martin,
        >
        >
        >
        > thank you for your comments! They triggered some deeper thoughts.
        >
        >
        >
        > I could not find any written proof that it is ok to write outside the bounds
        > given by the SP. The risk is minimal, though. You would need an external
        > interrupt to hit the gap between the write and the SP update.
        >
        >
        >
        > There is more than one code location affected. Doing it right will
        > definitely blow up the fix size. There is not enough time right now: I will
        > leave the office for vacation in an hour or so. I will resume working on the
        > issue once I’m back (June 19th).
        >
        >
        >
        > Best regards,
        >
        > Lutz
        >
        >
        >
        > On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:
        >
        >
        >
        > Hi Lutz,
        >
        >
        >
        > thanks for providing the webrev.
        >
        >
        >
        > Is it allowed to write to the stack before updating the SP?
        >
        > I know that the PPC ABI allows this within a certain range, but I’m not
        > aware of such an exception on s390x.
        >
        >
        >
        > I’d also prefer separate functions instead of one with so many cases.
        >
        > E.g. one function which copies the fp and one which takes a given fp like:
        >
        > void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {
        >
        >   assert_different_registers(newSP, fp, Z_SP);
        >
        >   z_lgr(Z_SP, newSP);
        >
        >   z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);
        >
        > }
        >
        >
        >
        > Thanks and best regards,
        >
        > Martin
        >
        >
        >
        >
        >
        > From: hotspot-compiler-dev
        > [mailto:[hidden email]] On Behalf Of Schmidt,
        > Lutz
        > Sent: Dienstag, 23. Mai 2017 12:47
        > To: [hidden email]
        > Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()
        >
        >
        >
        > Dear all,
        >
        >
        >
        > I would like to request reviews for this tiny, s390-only enhancement:
        >
        > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
        >
        > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.00/
        >
        >
        >
        > Thank you!
        >
        > Lutz
        >
        >
       
   
   
   
   
   
   

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

Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Schmidt, Lutz
Hi Martin,

after additional testing and some offline discussion I withdraw my “do not agree”.
In g1_write_barrier_pre, it is possible to omit lgr_if_needed(Rpre_val, Rpre_save) and pass Rpre_save instead of Rpre_val to the runtime call.

Thank you and Best Regards,
Lutz

 

On 26.07.2017, 12:58, "hotspot-compiler-dev on behalf of Schmidt, Lutz" <[hidden email] on behalf of [hidden email]> wrote:

    Hi Martin,
   
    thank you for reviewing my change. Please see my “>>>” inline comments below.
   
    Please find the new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8180659.03/
   
    Regards,
    Lutz
   
     
    Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
   
     
   
    On 25.07.2017, 19:25, "Doerr, Martin" <[hidden email]> wrote:
   
        Hi Lutz,
       
        Thanks for doing all the work. I have a couple of small change requests:
       
        macroAssembler_s390.cpp:
        push_frame:
        - z_stg(old_sp,... can be moved out of the if ... else ...
    >>> done.
       
        g1_write_barrier_pre:
        - Wouldn't assert or guarantee be better than z_SIGSEGV?
    >>> agree & changed. Z_SIGSEGV() was a leftover.
        - lgr_if_needed(Rpre_val, Rpre_save); can be replaced by using Rpre_save in the succeeding call_VM_leaf.
    >>> do not agree (see comment on Rpre_val above g1_write_barrier_pre).
       
        verify_oop:
        - I think nbytes_save should be 5*BytesPerWord because we're saving 5 regs. The "+ BytesPerWord" should not be needed. (Stack alignment on s390 is only 8 Bytes if I remember correctly.)
    >>> intention was to change stack layout as little as possible. The previous code saved Z_R0 as well – which is nonsense now. But OK. Modified to use only five slots.
       
       
        stubGenerator_s390.cpp:
        - Some comments mix up "implicitly done in save_live_registers" and "implicitly done in restore_live_registers".
        - When fixing this, you could also replace the double semicolons.
    >>> good catch! Comments adapted. Changed ;; -> ;.
       
        The rest looks good. Also, thanks for doing some cleanup.
       
        I'm ok with handling generate_push_parmBlk() in the separate bug.
       
        Btw. please keep the "[s390]" on the right side of the bug id and ":" in the email headline for future RFRs. Otherwise, it gets easily lost in hg log when pushing.
       
        Best regards,
        Martin
       
       
        -----Original Message-----
        From: Schmidt, Lutz
        Sent: Mittwoch, 12. Juli 2017 10:36
        To: Volker Simonis <[hidden email]>
        Cc: Doerr, Martin <[hidden email]>; [hidden email]
        Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()
       
        Thank you, Volker, for your in-depth review.
       
        As we can never be sure what the future brings, and because we are always short on registers on s390, I opt for “take the assertion out”. Done.
       
        Well observed! There is one place left (in generate_push_parmBlk()) where we access the stack outside the bounds given by Z_SP. I left the code “as is” for now. With the fix for JDK-8180823 (ready for review as soon as this one is through), the out-of-bounds access will go away.
       
        While re-testing, I ran into crashes which were obviously related to my changes and which I had not seen before. Analyzing the crashes and finding/fixing the root cause took quite a while: push_frame() now has to use a scratch register, and two callers relied on that scratch register to remain unaltered.
       
        The issue is solved, all tests we run inhouse pass and I have created a new webrev:
        http://cr.openjdk.java.net/~lucy/webrevs/8180659.02/ 
       
        Please have a (hopefully final) look. Thank you!
       
        Regards,
        Lutz
       
         
       
        On 30.06.2017, 11:37, "Volker Simonis" <[hidden email]> wrote:
       
            Hi Lutz,
           
            the change looks good - I just have tow small questions/comments:
           
            2050   assert_different_registers(newSP, Z_R0); // required to
            generate efficient code.
            ...
            2056   z_lgr(Z_SP, newSP);
            2057   if (newSP != Z_R0) { // despite the assert above, make sure we
            generate correct code.
           
            I think either we are sure that we don't call resize_frame_absolute()
            with 'newSP == Z_R0' (I think we currently can be as I've checked all
            the call sites) and keep just the assertion in order to check against
            future violations of this assumption. Or we think there will be
            future, valid uses of resize_frame_absolute() with 'newSP == Z_R0' in
            which case we should remove the assertion and keep the alternate code
            generation path.
           
            Also, as far as I understand, this change should fix writes below the
            SP, but there are still other places (notably in
            generate_push_parmBlk()) where we write below the SP before calling
            resize_frame_absolute() to adjust it. Will you fix them in a follow-up
            change?
           
            Thanks for fixing this,
            Volker
           
           
           
            On Thu, Jun 29, 2017 at 12:14 PM, Schmidt, Lutz <[hidden email]> wrote:
            > Thanks, Martin!
            >
            >
            >
            > I have renamed that one function and the copyright was updated as well. The
            > webrev was updated in-place.
            >
            >
            >
            > Regards, Lutz
            >
            >
            >
            > On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:
            >
            >
            >
            > Hi Lutz,
            >
            >
            >
            > nice change. Thanks for fixing writes below SP and also for improving
            > readability.
            >
            >
            >
            > I think the new pop_frame function should better be called e.g.
            > pop_frame_and_restore_retPC because it performs this additional step in
            > contrast to the other pop_frame function.
            >
            >
            >
            > s390.ad needs a copyright update.
            >
            >
            >
            > Besides that, it looks good to me. I can sponsor this change.
            >
            >
            >
            > Best regards,
            >
            > Martin
            >
            >
            >
            >
            >
            >
            >
            > From: Schmidt, Lutz
            > Sent: Donnerstag, 29. Juni 2017 10:48
            > To: Doerr, Martin <[hidden email]>;
            > [hidden email]
            > Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in
            > resize_frame_absolute()
            >
            >
            >
            > Hi Martin et al,
            >
            >
            >
            > Finally, here is the expanded rework of my change. Doesn’t look big, but it
            > took some time to make sure I didn’t miss places that need adaptation.
            >
            >
            >
            > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
            >
            > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.01/
            >
            >
            >
            > Please have a look.
            >
            >
            >
            > Thanks, Lutz
            >
            >
            >
            > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
            >
            >
            >
            >
            >
            > On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:
            >
            >
            >
            > Hi Martin,
            >
            >
            >
            > thank you for your comments! They triggered some deeper thoughts.
            >
            >
            >
            > I could not find any written proof that it is ok to write outside the bounds
            > given by the SP. The risk is minimal, though. You would need an external
            > interrupt to hit the gap between the write and the SP update.
            >
            >
            >
            > There is more than one code location affected. Doing it right will
            > definitely blow up the fix size. There is not enough time right now: I will
            > leave the office for vacation in an hour or so. I will resume working on the
            > issue once I’m back (June 19th).
            >
            >
            >
            > Best regards,
            >
            > Lutz
            >
            >
            >
            > On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:
            >
            >
            >
            > Hi Lutz,
            >
            >
            >
            > thanks for providing the webrev.
            >
            >
            >
            > Is it allowed to write to the stack before updating the SP?
            >
            > I know that the PPC ABI allows this within a certain range, but I’m not
            > aware of such an exception on s390x.
            >
            >
            >
            > I’d also prefer separate functions instead of one with so many cases.
            >
            > E.g. one function which copies the fp and one which takes a given fp like:
            >
            > void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {
            >
            >   assert_different_registers(newSP, fp, Z_SP);
            >
            >   z_lgr(Z_SP, newSP);
            >
            >   z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);
            >
            > }
            >
            >
            >
            > Thanks and best regards,
            >
            > Martin
            >
            >
            >
            >
            >
            > From: hotspot-compiler-dev
            > [mailto:[hidden email]] On Behalf Of Schmidt,
            > Lutz
            > Sent: Dienstag, 23. Mai 2017 12:47
            > To: [hidden email]
            > Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()
            >
            >
            >
            > Dear all,
            >
            >
            >
            > I would like to request reviews for this tiny, s390-only enhancement:
            >
            > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
            >
            > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.00/
            >
            >
            >
            > Thank you!
            >
            > Lutz
            >
            >
           
       
       
       
       
       
       
   
   

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

RE: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Doerr, Martin
Hi Lutz,

thanks for the confirmation. I've pushed your webrev 8180659.03 with this minor change.
Thanks again for doing all the work.

Best regards,
Martin


-----Original Message-----
From: Schmidt, Lutz
Sent: Donnerstag, 27. Juli 2017 15:34
To: Doerr, Martin <[hidden email]>; Volker Simonis <[hidden email]>
Cc: [hidden email]
Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()

Hi Martin,

after additional testing and some offline discussion I withdraw my “do not agree”.
In g1_write_barrier_pre, it is possible to omit lgr_if_needed(Rpre_val, Rpre_save) and pass Rpre_save instead of Rpre_val to the runtime call.

Thank you and Best Regards,
Lutz

 

On 26.07.2017, 12:58, "hotspot-compiler-dev on behalf of Schmidt, Lutz" <[hidden email] on behalf of [hidden email]> wrote:

    Hi Martin,
   
    thank you for reviewing my change. Please see my “>>>” inline comments below.
   
    Please find the new webrev at http://cr.openjdk.java.net/~lucy/webrevs/8180659.03/
   
    Regards,
    Lutz
   
     
    Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
   
     
   
    On 25.07.2017, 19:25, "Doerr, Martin" <[hidden email]> wrote:
   
        Hi Lutz,
       
        Thanks for doing all the work. I have a couple of small change requests:
       
        macroAssembler_s390.cpp:
        push_frame:
        - z_stg(old_sp,... can be moved out of the if ... else ...
    >>> done.
       
        g1_write_barrier_pre:
        - Wouldn't assert or guarantee be better than z_SIGSEGV?
    >>> agree & changed. Z_SIGSEGV() was a leftover.
        - lgr_if_needed(Rpre_val, Rpre_save); can be replaced by using Rpre_save in the succeeding call_VM_leaf.
    >>> do not agree (see comment on Rpre_val above g1_write_barrier_pre).
       
        verify_oop:
        - I think nbytes_save should be 5*BytesPerWord because we're saving 5 regs. The "+ BytesPerWord" should not be needed. (Stack alignment on s390 is only 8 Bytes if I remember correctly.)
    >>> intention was to change stack layout as little as possible. The previous code saved Z_R0 as well – which is nonsense now. But OK. Modified to use only five slots.
       
       
        stubGenerator_s390.cpp:
        - Some comments mix up "implicitly done in save_live_registers" and "implicitly done in restore_live_registers".
        - When fixing this, you could also replace the double semicolons.
    >>> good catch! Comments adapted. Changed ;; -> ;.
       
        The rest looks good. Also, thanks for doing some cleanup.
       
        I'm ok with handling generate_push_parmBlk() in the separate bug.
       
        Btw. please keep the "[s390]" on the right side of the bug id and ":" in the email headline for future RFRs. Otherwise, it gets easily lost in hg log when pushing.
       
        Best regards,
        Martin
       
       
        -----Original Message-----
        From: Schmidt, Lutz
        Sent: Mittwoch, 12. Juli 2017 10:36
        To: Volker Simonis <[hidden email]>
        Cc: Doerr, Martin <[hidden email]>; [hidden email]
        Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()
       
        Thank you, Volker, for your in-depth review.
       
        As we can never be sure what the future brings, and because we are always short on registers on s390, I opt for “take the assertion out”. Done.
       
        Well observed! There is one place left (in generate_push_parmBlk()) where we access the stack outside the bounds given by Z_SP. I left the code “as is” for now. With the fix for JDK-8180823 (ready for review as soon as this one is through), the out-of-bounds access will go away.
       
        While re-testing, I ran into crashes which were obviously related to my changes and which I had not seen before. Analyzing the crashes and finding/fixing the root cause took quite a while: push_frame() now has to use a scratch register, and two callers relied on that scratch register to remain unaltered.
       
        The issue is solved, all tests we run inhouse pass and I have created a new webrev:
        http://cr.openjdk.java.net/~lucy/webrevs/8180659.02/ 
       
        Please have a (hopefully final) look. Thank you!
       
        Regards,
        Lutz
       
         
       
        On 30.06.2017, 11:37, "Volker Simonis" <[hidden email]> wrote:
       
            Hi Lutz,
           
            the change looks good - I just have tow small questions/comments:
           
            2050   assert_different_registers(newSP, Z_R0); // required to
            generate efficient code.
            ...
            2056   z_lgr(Z_SP, newSP);
            2057   if (newSP != Z_R0) { // despite the assert above, make sure we
            generate correct code.
           
            I think either we are sure that we don't call resize_frame_absolute()
            with 'newSP == Z_R0' (I think we currently can be as I've checked all
            the call sites) and keep just the assertion in order to check against
            future violations of this assumption. Or we think there will be
            future, valid uses of resize_frame_absolute() with 'newSP == Z_R0' in
            which case we should remove the assertion and keep the alternate code
            generation path.
           
            Also, as far as I understand, this change should fix writes below the
            SP, but there are still other places (notably in
            generate_push_parmBlk()) where we write below the SP before calling
            resize_frame_absolute() to adjust it. Will you fix them in a follow-up
            change?
           
            Thanks for fixing this,
            Volker
           
           
           
            On Thu, Jun 29, 2017 at 12:14 PM, Schmidt, Lutz <[hidden email]> wrote:
            > Thanks, Martin!
            >
            >
            >
            > I have renamed that one function and the copyright was updated as well. The
            > webrev was updated in-place.
            >
            >
            >
            > Regards, Lutz
            >
            >
            >
            > On 29.06.2017, 11:40, "Doerr, Martin" <[hidden email]> wrote:
            >
            >
            >
            > Hi Lutz,
            >
            >
            >
            > nice change. Thanks for fixing writes below SP and also for improving
            > readability.
            >
            >
            >
            > I think the new pop_frame function should better be called e.g.
            > pop_frame_and_restore_retPC because it performs this additional step in
            > contrast to the other pop_frame function.
            >
            >
            >
            > s390.ad needs a copyright update.
            >
            >
            >
            > Besides that, it looks good to me. I can sponsor this change.
            >
            >
            >
            > Best regards,
            >
            > Martin
            >
            >
            >
            >
            >
            >
            >
            > From: Schmidt, Lutz
            > Sent: Donnerstag, 29. Juni 2017 10:48
            > To: Doerr, Martin <[hidden email]>;
            > [hidden email]
            > Subject: Re: [s390] RFR(XS): 8180659: micro-optimization in
            > resize_frame_absolute()
            >
            >
            >
            > Hi Martin et al,
            >
            >
            >
            > Finally, here is the expanded rework of my change. Doesn’t look big, but it
            > took some time to make sure I didn’t miss places that need adaptation.
            >
            >
            >
            > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
            >
            > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.01/
            >
            >
            >
            > Please have a look.
            >
            >
            >
            > Thanks, Lutz
            >
            >
            >
            > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
            >
            >
            >
            >
            >
            > On 01.06.2017, 16:58, "Schmidt, Lutz" <[hidden email]> wrote:
            >
            >
            >
            > Hi Martin,
            >
            >
            >
            > thank you for your comments! They triggered some deeper thoughts.
            >
            >
            >
            > I could not find any written proof that it is ok to write outside the bounds
            > given by the SP. The risk is minimal, though. You would need an external
            > interrupt to hit the gap between the write and the SP update.
            >
            >
            >
            > There is more than one code location affected. Doing it right will
            > definitely blow up the fix size. There is not enough time right now: I will
            > leave the office for vacation in an hour or so. I will resume working on the
            > issue once I’m back (June 19th).
            >
            >
            >
            > Best regards,
            >
            > Lutz
            >
            >
            >
            > On 29.05.2017, 12:58, "Doerr, Martin" <[hidden email]> wrote:
            >
            >
            >
            > Hi Lutz,
            >
            >
            >
            > thanks for providing the webrev.
            >
            >
            >
            > Is it allowed to write to the stack before updating the SP?
            >
            > I know that the PPC ABI allows this within a certain range, but I’m not
            > aware of such an exception on s390x.
            >
            >
            >
            > I’d also prefer separate functions instead of one with so many cases.
            >
            > E.g. one function which copies the fp and one which takes a given fp like:
            >
            > void MacroAssembler::resize_frame_absolute(Register newSP, Register fp) {
            >
            >   assert_different_registers(newSP, fp, Z_SP);
            >
            >   z_lgr(Z_SP, newSP);
            >
            >   z_stg(fp, _z_abi(callers_sp), (newSP == Z_R0) ? Z_SP : newSP);
            >
            > }
            >
            >
            >
            > Thanks and best regards,
            >
            > Martin
            >
            >
            >
            >
            >
            > From: hotspot-compiler-dev
            > [mailto:[hidden email]] On Behalf Of Schmidt,
            > Lutz
            > Sent: Dienstag, 23. Mai 2017 12:47
            > To: [hidden email]
            > Subject: [10] [s390] RFR(XS): micro-optimization in resize_frame_absolute()
            >
            >
            >
            > Dear all,
            >
            >
            >
            > I would like to request reviews for this tiny, s390-only enhancement:
            >
            > Bug:    https://bugs.openjdk.java.net/browse/JDK-8180659
            >
            > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8180659.00/
            >
            >
            >
            > Thank you!
            >
            > Lutz
            >
            >
           
       
       
       
       
       
       
   
   

Loading...