RFR 8144534: Refactor templateInterpreter and templateInterpreterGenerator functions

[Christian Thalinger]

> On Dec 4, 2015, at 10:49 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> Hi Chris,
> Thank you for looking at this.  Debt almost repaid.

Good :-)

> 
> On 12/4/15 12:48 PM, Christian Thalinger wrote:
>>> On Dec 3, 2015, at 1:51 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>> 
>>> 
>>> Hi Goetz,
>>> 
>>> Thank you for looking at my change.  I made the ppc changes and hg added templateInterpreter_ppc/aarch64.cpp back, so they should be in the patch.
>>> 
>>> Now webrev thinks I've deleted 7785 lines, but I didn't really.
>>> 
>>> http://cr.openjdk.java.net/~coleenp/8144534.02/
>>> 
>>> The most interesting part of this change is in the x86 code:
>>> 
>>> http://cr.openjdk.java.net/~coleenp/8144534.02/src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp.udiff.html
>>> 
>>> This is where the 32 and 64 bit template interpreter code is merged.
>> +   Register rarg = NOT_LP64(rax) LP64_ONLY(c_rarg1);
>> +   Register rarg2 = NOT_LP64(rbx) LP64_ONLY(c_rarg2);
>> 
>> Aren’t these always the same and could also be defined globally?  Maybe keep rarg1.
> 
> No, I don't think we usually use rbx for 32 bit.   I have other instances that we pick rdx and rbx.

Ok.

> 
>> 
>> !   // compute beginning of parameters (rdi/r14)
>> !   __ lea(rlocals, Address(rsp, rcx, Interpreter::stackElementScale(), -wordSize));
>> 
>> You can remove the rdi/r14 part.
> 
> done.
> 
>> 
>> !   // restore rsi/r13 to have legal interpreter frame, i.e., bci == 0 <=>
>>     // r13 == code_base()
>> 
>>     // compute beginning of parameters (r14)
>> 
>> Same for these.
>  // restore to have legal interpreter frame, i.e., bci == 0 <=> code_base()
> 
> Much better.
> 
>> 
>>       ExternalAddress unsatisfied(SharedRuntime::native_method_throw_unsatisfied_link_error_entry());
>> !     __ movptr(rscratch2, unsatisfied.addr());
>>       __ cmpptr(rax, rscratch2);
>> !     __ cmpptr(rax, unsatisfied.addr());
>> 
>> I think this is wrong.  On 64-bit cmp can’t take a 64-bit immediate.  That’s why we have the move.
> 
> Wow, you're right cmpptr is a macroAssembler instruction because it does the mov depending on the address.   We use cmpptr(reg, Address) all over.
> 
> 
> Thanks!
> Coleen
>> 
>> Otherwise this looks good but I’ve only looked at this one file.
>> 
>>> Thanks,
>>> Coleen
>>> 
>>> On 12/3/15 8:14 AM, Lindenmaier, Goetz wrote:
>>>> Hi Coleen,
>>>> 
>>>> I've been looking at this change. A cleanup in this area is really
>>>> useful.
>>>> I missed templateInterpreter_ppc.cpp in your patch, I guess you forgot
>>>> "hg add" for it.
>>>> In addition, I had to fix some code around math_entry_available(), see this patch:
>>>> http://cr.openjdk.java.net/~goetz/webrevs/8144534-interp/8144534-fixes_for_ppc.patch
>>>> 
>>>> In general, I think it's strange that AbstractInterpreterGenerator
>>>> implementations are in interpreter_<cpu>.cpp, as mentioned In the bug.
>>>> Why not move them to templateInterpreterGenerator_<cpu>.cpp? Or will
>>>> this be subject to a later change that removes the funny common
>>>> subclasses Interpreter/InterpreterGenerator?
>>>> 
>>>> Should I make a change removing the CC_INTERP support from ppc?
>>>> 
>>>> Best regards,
>>>>   Goetz.
>>>> 
>>>>> -----Original Message-----
>>>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On
>>>>> Behalf Of Coleen Phillimore
>>>>> Sent: Donnerstag, 3. Dezember 2015 02:58
>>>>> To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>>>>> Subject: RFR 8144534: Refactor templateInterpreter and
>>>>> templateInterpreterGenerator functions
>>>>> 
>>>>> Summary: merged templateInterpreter_x86_32/64 into
>>>>> templateInterpreterGenerator_x86.cpp (some 32/64 functions remain for
>>>>> the hand coded crc functions).
>>>>> 
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8144534.01/
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8144534
>>>>> 
>>>>> I tried to make this reviewable by hg renaming files.   I basically
>>>>> renamed templateInterpreter_ppc.cpp and removed the
>>>>> AbstractInterpreter
>>>>> functions and put them back into templateInterpreter_ppc.cpp.  I didn't
>>>>> really delete 4000 lines of code, more like 1500.
>>>>> 
>>>>> Also, can someone with the PPC and AARCH port look at and test out these
>>>>> changes? I tried to reduce the #includes in the new
>>>>> templateInterpreter_ppc/aarch64.cpp files which worked for sparc.
>>>>> 
>>>>> Tested with JPRT on all platforms, also ran jtreg and
>>>>> collocated/non-collocated tonga tests for linux x64 and i386 since
>>>>> that's were the real changes are.
>>>>> 
>>>>> See the bug for more details and my partial vision of how these
>>>>> functions will be reorganized when I remove the broken c++ interpreter.
>>>>> 
>>>>> Also tested that Zero still builds.
>>>>> 
>>>>> Thanks!
>>>>> Coleen