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

Thu Jan 7 08:08:11 UTC 2016

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
> >