RFR(M) 8155980: ARM InterpreterMacroAssembler::get_method_counters() should not be saving callee saved registers

Tue Jan 17 22:30:09 UTC 2017

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.

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);
Thanks,
Jiangli

> On Jan 13, 2017, at 9:31 AM, Chris Plummer <chris.plummer at oracle.com> wrote:
> 
> Here's an improvement on the second solution:
> 
> http://cr.openjdk.java.net/~cjplummer/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
>> 
>> 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
>> 
>> 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 at 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
>>>>>> 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
>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 