RFR (M) 8146410: Interpreter functions are declared and defined in the wrong files
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Jan 8 09:57:35 UTC 2016
Hi Coleen,
this change works fine, thanks!
Best regards,
Goetz.
> -----Original Message-----
> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> Sent: Donnerstag, 7. Januar 2016 18:54
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> dev at openjdk.java.net
> Subject: Re: RFR (M) 8146410: Interpreter functions are declared and defined
> in the wrong files
>
>
> 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