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

[Chris Plummer]

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