[EXTERNAL] Re: RFR(M): 8248190: PPC: Enable Power10 system and use new byte-reverse instructions

[Doerr, Martin]

Hi Jose,

you already have 2 reviews by JDK Reviewers.
The change needs to get the formal information including "Reviewed-by" and "Contributed-by" information added such that it passes jcheck. Then you only need a sponsor to push it. I guess Michihiro wants to do that for you?

Best regards,
Martin


> -----Original Message-----
> From: joserz at linux.ibm.com <joserz at linux.ibm.com>
> Sent: Montag, 24. August 2020 14:36
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: Thomas Schatzl <thomas.schatzl at oracle.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: Re: [EXTERNAL] Re: RFR(M): 8248190: PPC: Enable Power10 system
> and use new byte-reverse instructions
> 
> Hallo Martin!
> 
> Just to understand. Do I need to do something else? Ask more reviewers?
> 
> Thank you :)
> 
> Jose
> 
> On Fri, Aug 21, 2020 at 03:25:46PM +0000, Doerr, Martin wrote:
> > Hi Thomas,
> >
> > I understand your point. My concern is that it may become a more political
> discussion how to handle CSR for PPC64 flags and I don't want to delay Jose's
> change for that. There are already other changes in the pipe which build on
> top of it.
> >
> > It will probably be us to handle and approve CSR requests for platforms
> which are maintained by SAP. We haven't done this so far. We are still
> handling such flags in a less formal way.
> > I don't know how other non-Oracle platforms are handled.
> >
> > Best regards,
> > Martin
> >
> >
> > > -----Original Message-----
> > > From: Thomas Schatzl <thomas.schatzl at oracle.com>
> > > Sent: Freitag, 21. August 2020 17:12
> > > To: Doerr, Martin <martin.doerr at sap.com>; joserz at linux.ibm.com
> > > Cc: hotspot-compiler-dev at openjdk.java.net
> > > Subject: Re: [EXTERNAL] Re: RFR(M): 8248190: PPC: Enable Power10
> system
> > > and use new byte-reverse instructions
> > >
> > > Hi,
> > >
> > > On 21.08.20 17:06, Doerr, Martin wrote:
> > > > Hi Thomas,
> > > >
> > > > I agree with you in general. However, all PPC64 specific platform flags
> are
> > > "product" at the moment.
> > > > Most of them should probably be "diagnostic". We should fix that at
> some
> > > point of time.
> > > > But for now, I'm ok with Jose's webrev since it's consistent with the
> other
> > > PPC64 flags.
> > > >
> > >
> > >    I was merely pointing out what the rule is, that has not been a veto
> > > for the patch (which I haven't reviewed btw). If you want to go ahead
> > > with that for consistency's sake, with a plan to fix this I can see your
> > > point of keeping it.
> > >
> > > Thanks,
> > >    Thomas
> > >
> > > > Best regards,
> > > > Martin
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: hotspot-compiler-dev <hotspot-compiler-dev-
> > > >> retn at openjdk.java.net> On Behalf Of Thomas Schatzl
> > > >> Sent: Freitag, 21. August 2020 15:45
> > > >> To: joserz at linux.ibm.com
> > > >> Cc: hotspot-compiler-dev at openjdk.java.net
> > > >> Subject: Re: [EXTERNAL] Re: RFR(M): 8248190: PPC: Enable Power10
> > > system
> > > >> and use new byte-reverse instructions
> > > >>
> > > >> Hi,
> > > >>
> > > >> On 21.08.20 15:37, joserz at linux.ibm.com wrote:
> > > >>> Hello!
> > > >>>
> > > >>> On Fri, Aug 21, 2020 at 10:04:38AM +0200, Thomas Schatzl wrote:
> > > >>>> Hi,
> > > >>>>
> > > >>>> On 21.08.20 04:33, Michihiro Horie wrote:
> > > >>>>>
> > > >>>>> Hi Jose,
> > > >>>>>
> > > >>>>> One thing I noticed is a misaligned backslash in globals_ppc.hpp.
> > > >>>>> Otherwise, the change looks good!
> > > >>>>>
> > > >>>>>       /* special instructions */
> > > >>>>> \
> > > >>>>> +  product(bool, UseByteReverseInstructions, false,
> > > >>>>> \
> > > >>>>
> > > >>>> Fwiw, for adding product options, you must go through the CSR
> > > process.
> > > >> Maybe
> > > >>>> there is an exception for platform specific ones?
> > > >>>
> > > >>> I didn't find any exception for platform specific options. But,
> > > >> "experimental" options
> > > >>> don't need such CSR process and, to be honest, experimental seems
> > > more
> > > >> appropriate here.
> > > >>> What do you think?
> > > >>>
> > > >>> Thank you for your review! :)
> > > >>
> > > >> Just a fly-by. It's up to you :) - just that product options need to be
> > > >> announced to the world.
> > > >>
> > > >> I kind of agree that experimental seems more appropriate. You can
> > > always
> > > >> "upgrade" it later.
> > > >>
> > > >> Thomas
> >