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

Thu Jan 7 17:53:37 UTC 2016

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