RFR (M) 8146410: Interpreter functions are declared and defined in the wrong files

Tue Jan 5 21:00:52 UTC 2016

On 1/5/16 3:49 PM, Lindenmaier, Goetz wrote:
>
>> -----Original Message-----
>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>> Sent: Dienstag, 5. Januar 2016 21:01
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-dev
>> developers <hotspot-dev at openjdk.java.net>
>> Subject: Re: RFR (M) 8146410: Interpreter functions are declared and defined
>> in the wrong files
>>
>>
>>
>> On 1/5/16 2:51 PM, Lindenmaier, Goetz wrote:
>>> Hi Coleen,
>>>
>>> I think they belong together because can_be_compiled() just returns
>>> false for functions that have a math entry which is generated by
>>> generate_math_entry().  So if you add a new math_entry, you
>>> also have to fix can_be_compiled.  We wired this dependency into
>>> math_entry_available(kind), so that both functions are guaranteed
>>> to behave similar.
>> I'm getting confused by this.  It seems like math_entry_available should
>> be in templateInterpreterGenerator near generate_math_entry.
>>
>> The reason I don't want can_be_compiled (which is misleading) is because
>> all the other platforms have it too and I don't want them in the wrong
>> files.
>>
>> what do you think?
> What I mean is can_be_compiled() should be moved to class
> templateInterpreterGenerator and placed near generate_math_entry()
> on all platforms.
> The similarity is the same on all of them.
>
> But I must admit in compilationPolicy.cpp it reads better if the
> AbstractInterpreter is asked. Also, method_kind is member
> of AbstractInterpreter.

Yes, that's what I thought.  It's more of an Interpreter:: method. And 
it's a static function and the functions in TemplateInterpreterGenerator 
are not generally static functions.  I didn't want to move the others there.

>
> But method_kind is also completely over-engeneered, as it's
> only used in can_be_compiled, and there it's only used
> for math entries...
>
> Maybe a change of it's own is needed...

I'm holding my head in pain, and my change is too big already to add 
this.  I did move math_entry_available because to 
templateInterpreterGenerator_ppc.cpp because there is no 
templateInterpreter_ppc.cpp anymore (there would only be this one 
function and the sizing).  I think this looks good and better than 
having TemplateInterpreter::math_entry_available() in 
abstractInterpreter_ppc.cpp.   See:

http://cr.openjdk.java.net/~coleenp/8146410.01/webrev/src/cpu/ppc/vm/templateInterpreterGenerator_ppc.cpp.udiff.html

I think moving can_be_compiled and cleaning up the method_kind vs. 
intrinsics is a bigger problem to solve, and should be attempted in a 
further RFE.

Thanks,
Coleen

>
> Best regards,
>    Goetz.
>
>
>
>
>
>
>
>> Coleen
>>> The name of the function is kind of misleading to me.
>>>
>>> But I won't insist...
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>>>> Sent: Tuesday, January 05, 2016 7:17 PM
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-dev
>>>> developers <hotspot-dev at openjdk.java.net>
>>>> Subject: Re: RFR (M) 8146410: Interpreter functions are declared and
>> defined
>>>> in the wrong files
>>>>
>>>>
>>>>
>>>> On 1/5/16 2:19 AM, Lindenmaier, Goetz wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Could you please also move
>>>>> AbstractInterpreter::can_be_compiled(methodHandle m)
>>>>> to above
>>>>>
>> TemplateInterpreterGenerator::generate_math_entry(AbstractInterpreter::
>>>> MethodKind kind)?
>>>>> I think these belong together.
>>>> Really?  They're in different classes (hence the motivation for the
>>>> change) and generate_math_entry doesn't call can_be_compiled.
>>>> CompilationPolicy does but it calls it from
>>>> AbstractInterpreter::can_be_compiled() so I really can't move this
>>>> function into TemplateInterpreterGenerator.  It doesn't make sense to
>> me.
>>>>> We seem to have different BIND macros on ppc. The '__' is also in the
>>>>> macro. Could you please fix this small issue? It breaks the ppc build.
>>>>>
>>>>> diff -r 743aa331fc90
>>>> src/cpu/ppc/vm/templateInterpreterGenerator_ppc.cpp
>>>>> --- a/src/cpu/ppc/vm/templateInterpreterGenerator_ppc.cpp	Tue
>>>> Jan 05 07:47:21 2016 +0100
>>>>> +++ b/src/cpu/ppc/vm/templateInterpreterGenerator_ppc.cpp	Tue
>>>> Jan 05 08:06:12 2016 +0100
>>>>> @@ -416,7 +416,7 @@
>>>>>       default: ShouldNotReachHere();
>>>>>       }
>>>>>
>>>>> -  __ BIND(done);
>>>>> +  BIND(done);
>>>>>       __ blr();
>>>>>
>>>> Fixed.  Thanks!!
>>>>>       return entry;
>>>>>
>>>>> Besides this, the change looks good.
>>>> Thanks,
>>>> Coleen
>>>>
>>>>> Thanks and best regards,
>>>>>      Goetz.
>>>>>
>>>>> PS: is it possible to share your Copyright script?
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net]
>> On
>>>>>> Behalf Of Coleen Phillimore
>>>>>> Sent: Monday, January 04, 2016 11:43 PM
>>>>>> To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>>>>>> Subject: RFR (M) 8146410: Interpreter functions are declared and
>> defined
>>>> in
>>>>>> the wrong files
>>>>>>
>>>>>> Summary: Moved functions to the correct files.
>>>>>>
>>>>>> See bug for more details.
>>>>>>
>>>>>> I basically did an hg mv templateInterpreter_<cpu>.cpp
>>>>>> abstractInterpreter_<cpu>.cpp and moved the interpreter_<cpu>.cpp
>>>>>> functions there.
>>>>>>
>>>>>> Also moved generate_slow_signature_handler to
>>>>>> TemplateInterpreterGenerator/CppInterpreterGenerator because it's
>>>> not
>>>>>> shared.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8146410/
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8146410
>>>>>>
>>>>>> Tested with JPRT on Oracle supported platforms and built zero on linux
>>>>>> x86.  Also fixed change that broke zero in stack_zero.inline.hpp.  I
>>>>>> think this should work on PPC and AARCH64, but please let me know.
>>>>>>
>>>>>> One question for AARCH64 platform in file:
>>>>>>
>>>>>>
>> http://cr.openjdk.java.net/~coleenp/8146410/src/cpu/aarch64/vm/templat
>>>>>> eInterpreterGenerator_aarch64.cpp.udiff.html
>>>>>>
>>>>>> thanks,
>>>>>> Coleen