RFR(S): 8212481: PPC64: Enable POWER9 CPU detection

[Volker Simonis]

Hi Gustavo,

thanks for adding Power9 support. Your change looks good!

Just two minor comments (no need to publish a new webrev):

- can you please update the copyright year to 2018 in
assembler_ppc.inline.hpp and vm_version_ppc.hpp
- in assembler_ppc.inline.hpp can you pleas add a comment with the
default value as shown below? I often find that useful when browsing
the code.

+// Deliver A Random Number (introduced with POWER9)
+inline void Assembler::darn(Register d, int l /* = 1 */) {
emit_int32( DARN_OPCODE | rt(d) | l14(l)); }
+

Thank you and best regards,
Volker

On Wed, Oct 24, 2018 at 3:35 PM Gustavo Romero
<gromero at linux.vnet.ibm.com> wrote:
>
> Hi Martin,
>
> On 10/24/2018 05:20 AM, Doerr, Martin wrote:
> > looks good. Thank you for cleaning things up and also for sharing the story about mftgpr. Interesting.
> > Reviewed.
>
> Thanks for reviewing!
>
>
> Best regards,
> Gustavo
>
> > Best regards,
> > Martin
> >
> >
> > -----Original Message-----
> > From: Gustavo Romero <gromero at linux.vnet.ibm.com>
> > Sent: Dienstag, 23. Oktober 2018 23:46
> > To: Doerr, Martin <martin.doerr at sap.com>; hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
> > Subject: Re: RFR(S): 8212481: PPC64: Enable POWER9 CPU detection
> >
> > Hi Martin,
> >
> > Thanks for your comments on fsqrt{,s} and mftgpr opcodes.
> >
> > On 10/23/2018 01:51 PM, Doerr, Martin wrote:
> >> Hi Gustavo,
> >>
> >>> I agree. So, if you don't mind, to reduce any future rework on that
> >>> I removed the hardcoded L=1 and now it's just a default. I introduced
> >>> l14() function that matches two bits in the proper field as I don't
> >>> see any collision with any other one bit field at the same position
> >>> (bit 14).
> >> I like it, but the default value needs to get specified in the .hpp file.
> >
> > Fixed.
> >
> >
> >>> I just have one question regarding the feature-string. I see
> >>> instrumentation for "fsqrts" feature but it's not currently
> >>> advertised by the feature-string. On the other hand, ISA info
> >>> looks to indicate that fsqrt and fsqrts are not aliases, because:
> >>>
> >>> fsqrt  P2  -> Instruction introduced in the POWER2 Architecture.
> >>> fsqrts PPC -> Instruction introduced in the PowerPC Architecture prior to ISA v2.00
> >>>
> >>> so I'm wondering if just for completeness the "fsqrts" feature
> >>> should be included in the feature-string. Besides that I don't
> >>> see instrumentation to support has_mftgpr(), which is commented
> >>> out, thus should we remove it? Like the following:
> >>
> >> This code is very old. If we are sure nobody can run the VM on a machine without these instructions they can get removed from the feature checking.
> >
> > I'm not sure about it. Maybe somebody is using out there, in the community
> > for example, so kept it.
> >
> >
> >> The mftgpr was only designed for an extension of Power6 and the opcode was reused for something else later on if I remember correctly. I think you can remove the remaining comment.
> >
> > I was not aware of that, thanks for the info.
> > Just out of curiosity I asked the toolchain folks and they said that mftgpr
> > is present in some POWER6 hardware but on the so-called raw mode (ie power6x,
> > which I think it's the extension you mentioned), but not in the architected /
> > normal mode (power6). So even if it's present in the hardware it's disabled
> > unless you boot the system up in raw mode.
> >
> > On that raw mode (power6x) one would see the following in AUXV, for instance:
> >
> > $ LD_SHOW_AUXV=1 /bin/true | grep PLATFORM
> > AT_PLATFORM:     power6x
> > AT_BASE_PLATFORM:power6x
> >
> > But probably theses machines w/ power6x never got out of IBM indeed.
> >
> > Anyway, they said that power6x is unsupported. So, as you said, I removed the
> > comment about mftgpr.
> >
> > v3 webrev:
> >
> > http://cr.openjdk.java.net/~gromero/8212481/v3/
> >
> >
> > Thanks!
> >
> > Best regards,
> > Gustavo
> >
>