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

Christian Thalinger christian.thalinger at oracle.com
Tue Jan 12 01:50:20 UTC 2016 

Assuming you copied the functions verbatim this looks good.  Good improvement.

> On Jan 7, 2016, at 7:53 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> Hi Goetz,
> 
> I messed up when I moved math_entry_available.   I meant to leave it in class TemplateInterpreter but move it to templateInterpreterGenerator_ppc.cpp file.
> 
> But that would defeat some of the purpose of this change to make sure that functions were in the right files.  I didn't like TemplateInterpreter::math_entry_available() in abstractInterpreter_ppc.cpp either though.
> 
> So, I moved the function math_entry_available into class AbstractInterpreter which is called by AbstractInterpreter::can_be_compiled(), hence avoiding a call to a derived class which didn't seem good.  This makes the most sense to me.
> 
> Can you check out:
> 
> http://cr.openjdk.java.net/~coleenp/8146410.02/webrev/index.html
> 
> thanks,
> Coleen
> 
> On 1/7/16 3:08 AM, Lindenmaier, Goetz wrote:
>> Hi Coleen,
>> 
>> sorry I didn't answer yesterday, we had another holiday.
>> 
>> You moved PPCs math_entry_available() in your new change.
>> I think it makes no difference where it's located, as long as it's
>> called from both, interpreter and interpreterGenerator classes.
>> 
>> But if you want to move it, you need to fix all the other occurances
>> of the method, too:
>> http://cr.openjdk.java.net/~goetz/wr16/8146410-interpCleanup/move_math_entry_available.patch
>> 
>> Elsewise good.
>> 
>> Best regards,
>>   Goetz.
>> 
>>> -----Original Message-----
>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On
>>> Behalf Of Coleen Phillimore
>>> Sent: Dienstag, 5. Januar 2016 22:20
>>> To: hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR (M) 8146410: Interpreter functions are declared and defined
>>> in the wrong files
>>> 
>>> 
>>> 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/t
>>> emplateInterpreterGenerator_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
>

More information about the hotspot-dev mailing list