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

[Chris Plummer]

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
>>>>
>>>>
>>>
>>
>