[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(M) 8155980: ARM InterpreterMacroAssembler::get_method_counters() should not be
From: Jiangli Zhou <jiangli.zhou () oracle ! com>
Date: 2017-01-17 23:11:16
Message-ID: DB73273E-B569-465C-B073-F20965CD919B () oracle ! com
[Download RAW message or body]
Hi Chris,
> On Jan 17, 2017, at 2:55 PM, Chris Plummer <chris.plummer@oracle.com> wrote:
>
> On 1/17/17 2:30 PM, Jiangli Zhou wrote:
> > Hi Chris,
> >
> > I like the second solution too. I'd suggest to add asserts in \
> > get_method_counters() to make sure reg1, reg2 and reg3 are valid registers (not \
> > noreg) when ‘saveRegs' is true.
> Ok.
> >
> > In TemplateTable::branch(), why Bumped_taken_count is only save/restored for \
> > AARCH64? The following usage does not seem to be AARCH64 only.
> > 2322 if (UseOnStackReplacement) {
> > 2323 // check for overflow against Rbumped_taken_count, which is the \
> > MDO taken count 2324 const Address backward_branch_limit(Rcounters, \
> > in_bytes(MethodCounters::interpreter_backward_branch_limit_offset())); 2325 \
> > __ ldr_s32(Rtemp, backward_branch_limit); 2326 __ \
> > cmp(Rbumped_taken_count, Rtemp); 2327 __ b(dispatch, lo);
> Because R5 is a callee saved registers for ARM32, but caller saved for ARM64.
Ok. Can you please add above information in the comment in case someone has the same \
question when looking at the code in the future.
Thanks,
Jiangli
>
> thanks,
>
> Chris
> > Thanks,
> > Jiangli
> >
> > > On Jan 13, 2017, at 9:31 AM, Chris Plummer <chris.plummer@oracle.com \
> > > <mailto:chris.plummer@oracle.com>> wrote:
> > > Here's an improvement on the second solution:
> > >
> > > http://cr.openjdk.java.net/~cjplummer/8155980/webrev.05/webrev.hotspot \
> > > <http://cr.openjdk.java.net/%7Ecjplummer/8155980/webrev.05/webrev.hotspot>
> > > I make use of "noreg" to clean up handling the fact that 64-bit has to save a \
> > > 3rd register and 32-bit does not.
> > > thanks,
> > >
> > > Chris
> > >
> > > On 1/11/17 9:53 AM, Chris Plummer wrote:
> > > > Here are a couple of different solutions:
> > > >
> > > > This first one uses an alternate branch label. It also adds saving \
> > > > Rbumped_taken_count on AARCH64. Changes since the first solution are only in \
> > > > templateTable_arm.cpp.
> > > > http://cr.openjdk.java.net/~cjplummer/8155980/webrev.03/webrev.hotspot \
> > > > <http://cr.openjdk.java.net/~cjplummer/8155980/webrev.03/webrev.hotspot>
> > > > This second one moves the save/restore logic into get_method_counters(). The \
> > > > registers to save have to be passed in. It also adds saving \
> > > > Rbumped_taken_count on AARCH64. Changes since the first solution are in \
> > > > templateTable_arm.cpp, interp_masm_arm.cpp, and interp_masm_arm.hpp.
> > > > http://cr.openjdk.java.net/~cjplummer/8155980/webrev.04/webrev.hotspot \
> > > > <http://cr.openjdk.java.net/~cjplummer/8155980/webrev.04/webrev.hotspot>
> > > > I think overall the second one looks cleaner. Save/restore logic is all in \
> > > > one place, and is only done if call_VM() generated code is actually executed. \
> > > > Possibly the handling of the Rbumped_taken_count could be better to make it \
> > > > clear it's just for AARCH64. Maybe I could use AARCH64_ONLY around the \
> > > > argument declaration and passing.
> > > > thanks,
> > > >
> > > > Chris
> > > >
> > > > On 1/9/17 12:18 PM, Chris Plummer wrote:
> > > > > Sigh, there's a bug in the following:
> > > > >
> > > > > #ifdef AARCH64
> > > > > __ stp(Rdisp, R3_bytecode, Address(Rstack_top, -2*wordSize, pre_indexed));
> > > > > #else
> > > > > __ push(RegisterSet(Rdisp) | RegisterSet(R3_bytecode));
> > > > > #endif
> > > > > __ get_method_counters(Rmethod, Rcounters, dispatch);
> > > > > #ifdef AARCH64
> > > > > __ ldp(Rdisp, R3_bytecode, Address(Rstack_top, 2*wordSize, post_indexed));
> > > > > #else
> > > > > __ pop(RegisterSet(Rdisp) | RegisterSet(R3_bytecode));
> > > > > #endif
> > > > >
> > > > > Note that the "dispatch" label is passed to get_method_counters(), and it \
> > > > > will generate code that branches to it, thus by passing the pop. Need to \
> > > > > rethink this. I might need to introduce a new label just before the \
> > > > > dispatch label that will do the pop. The other choice is possibly \
> > > > > get_method_counters() should be told which registers to save.
> > > > > BTW, while looking over this code more carefully, it looks like for \
> > > > > AARCH64, r5 also needs to be saved and restored. It's volatile on AARCH64, \
> > > > > and is being used to store Rbumped_taken_count.
> > > > > Chris
> > > > >
> > > > > On 1/9/17 9:08 AM, Chris Plummer wrote:
> > > > > > Thanks Dean. Unfortunately I got a crash over the weekend when repeatedly \
> > > > > > running one of our test suites. Happens about 1 out of 10 times on both \
> > > > > > 32-bit and 64-bit. Looking into it now.
> > > > > > Chris
> > > > > >
> > > > > > On 1/6/17 3:57 PM, dean.long@oracle.com <mailto:dean.long@oracle.com> \
> > > > > > wrote:
> > > > > > > It looks good but you might want to add a comment in \
> > > > > > > TemplateTable::branch() saying something like:
> > > > > > > // save and restore caller-saved registers that will be trashed by \
> > > > > > > call_VM
> > > > > > > dl
> > > > > > >
> > > > > > >
> > > > > > > On 1/6/17 3:14 PM, Chris Plummer wrote:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > Please review the following arm changes. They will be pushed to JDK \
> > > > > > > > 10 only (when it opens).
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8155980 \
> > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8155980> \
> > > > > > > > http://cr.openjdk.java.net/~cjplummer/8155980/webrev.02/webrev.hotspot \
> > > > > > > > <http://cr.openjdk.java.net/~cjplummer/8155980/webrev.02/webrev.hotspot> \
> > > > > > > >
> > > > > > > > JDK-8012544 added a bunch of callee register saving to \
> > > > > > > > InterpreterMacroAssembler::get_method_counters() around the call_VM() \
> > > > > > > > call. The reason is because there are a couple of callers of \
> > > > > > > > get_method_counters() that need to have r0 and r3 preserved. \
> > > > > > > > get_method_counters() should not be responsible for having to save \
> > > > > > > > callee saved registers. It does not know which ones are in use when \
> > > > > > > > it is called, so it can't do this efficiently. In fact 2 of the 4 \
> > > > > > > > callers of get_method_counters() do not need any registers saved. For \
> > > > > > > > this reason the callee of get_method_counters() should be responsible \
> > > > > > > > for saving callee registers.
> > > > > > > > This solution would have been been pretty straight forward, except \
> > > > > > > > that AARCH64 does not allow SP to go above extended_sp, so when \
> > > > > > > > setting up extended_sp I needed to make sure there will be room for \
> > > > > > > > the 2 registers that need to be pushed. extended_sp is mainly based \
> > > > > > > > on max_stack() for the method, plus an extra slot for jsr292 and \
> > > > > > > > exception handling (but not both at the same time). So the fix here \
> > > > > > > > is mostly about making sure there are always at least 2 extra slots \
> > > > > > > > for pushing the 2 registers.
> > > > > > > > Here are the changes:
> > > > > > > >
> > > > > > > > interp_masm_arm.cpp
> > > > > > > > -No longer save/restore registers in get_method_counters():
> > > > > > > >
> > > > > > > > templateTable_arm.cpp:
> > > > > > > > -Save/restore Rdisp and R3_bytecode to stack around calls to \
> > > > > > > > get_method_counters.
> > > > > > > > abstractinterpreter__arm.cpp::
> > > > > > > > -Properly account for extra 2 slots needed on AARCH64 when creating a \
> > > > > > > > frame in layout_activation()
> > > > > > > > -Note I switched to using method->constMethod()->max_stack() because
> > > > > > > > method->max_stack() includes Method::extra_stack_entries(), and I \
> > > > > > > > want to account for Method::extra_stack_entries() separately.
> > > > > > > >
> > > > > > > > templateInterpreterGenerator_arm.cpp:
> > > > > > > > -Properly account for extra 2 slots needed on AARCH64 when creating a \
> > > > > > > > frame in generate_normal_entry().
> > > > > > > >
> > > > > > > > I've tested the following with -Xcomp and with -Xmixed on arm32 and \
> > > > > > > > arm64:
> > > > > > > > Kitchensink
> > > > > > > > vm.mlvm
> > > > > > > > > jdk_svc
> > > > > > > > > hotspot_runtime
> > > > > > > > > hotspot_serviceability
> > > > > > > > > hotspot_compiler
> > > > > > > > > jdk_lang
> > > > > > > > > jdk_util
> > > > > > > >
> > > > > > > > I also tested the test case from JDK-8012544.
> > > > > > > >
> > > > > > > > thanks,
> > > > > > > >
> > > > > > > > Chris
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic