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

Tue Jan 5 21:19:33 UTC 2016

Hi,  Please see updated webrev for comments so far:

http://cr.openjdk.java.net/~coleenp/8146410.01/webrev/

Tested again with JPRT (on all Oracle supported platforms).

thanks,
Coleen

On 1/5/16 4:00 PM, Coleen Phillimore wrote:
>
>
> 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
>