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

Coleen Phillimore coleen.phillimore at oracle.com
Tue Jan 12 18:09:13 UTC 2016 


On 1/11/16 8:50 PM, Christian Thalinger wrote:
> Assuming you copied the functions verbatim this looks good.  Good improvement.

Yes, they are only copied.

Thanks, Christian!
Coleen
>
>> 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