[s390] RFR(XS): 8180659: micro-optimization in resize_frame_absolute()
Schmidt, Lutz
lutz.schmidt at sap.com
Wed Jul 26 10:58:30 UTC 2017
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" <martin.doerr at sap.com> 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 <volker.simonis at gmail.com>
Cc: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-dev at openjdk.java.net
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" <volker.simonis at gmail.com> 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 <lutz.schmidt at sap.com> 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" <martin.doerr at sap.com> 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 <martin.doerr at sap.com>;
> hotspot-compiler-dev at openjdk.java.net
> 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" <lutz.schmidt at sap.com> 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" <martin.doerr at sap.com> 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:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Schmidt,
> Lutz
> Sent: Dienstag, 23. Mai 2017 12:47
> To: hotspot-compiler-dev at openjdk.java.net
> 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
>
>
More information about the hotspot-compiler-dev
mailing list