RFR: 8208171: PPC64: Enrich SLP support
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Sep 3 12:27:56 UTC 2018
Hi Michihiro,
I had a look at your change.
First, this should have been reviewed on hotspot-compiler-dev. It is clearly
a compiler change.
http://mail.openjdk.java.net/mailman/listinfo says that hotspot-dev is for
"Technical discussion about the development of the HotSpot virtual machine that's not specific to any particular component"
while hotspot-compiler-dev is for
"Technical discussion about the development of the HotSpot bytecode compilers"
Also, I can not find all of the mail traffic in
http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/thread.html.
Is this a problem of the pipermail server?
For some reason this webrev lacks the links to browse the diffs.
Do you need to use a more recent webrev? You can obtain it with
hg clone http://hg.openjdk.java.net/code-tools/webrev/ .
Why do you rename vnoreg to vnoregi?
Besides that the change is fine, thanks for implementing this!
Best regards,
Goetz.
> -----Original Message-----
> From: Doerr, Martin
> Sent: Dienstag, 28. August 2018 19:35
> To: Gustavo Romero <gromero at linux.vnet.ibm.com>; Michihiro Horie
> <HORIE at jp.ibm.com>
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net; Simonis, Volker
> <volker.simonis at sap.com>
> Subject: RE: RFR: 8208171: PPC64: Enrich SLP support
>
> Hi Michihiro,
>
> thank you for implementing it. I have just taken a first look at your
> webrev.01.
>
> It looks basically good. Only the Power version check seems to be incorrect.
> VM_Version::has_popcntb() checks for Power5.
> I believe most instructions are available with Power7.
> Some ones (vsubudm, ..., vmmuluwm, vpopcntw) were introduced with
> Power8?
> We should check this carefully.
>
> Also, indentation in register_ppc.hpp could get improved.
>
> Thanks and best regard,
> Martin
>
>
> -----Original Message-----
> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
> Sent: Donnerstag, 26. Juli 2018 16:02
> To: Michihiro Horie <HORIE at jp.ibm.com>
> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> dev at openjdk.java.net; Doerr, Martin <martin.doerr at sap.com>; ppc-aix-
> port-dev at openjdk.java.net; Simonis, Volker <volker.simonis at sap.com>
> Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
>
> Hi Michi,
>
> On 07/26/2018 01:43 AM, Michihiro Horie wrote:
> > I updated webrev:
> > http://cr.openjdk.java.net/~mhorie/8208171/webrev.01/
>
> Thanks for providing an updated webrev and for fixing indentation and
> function
> order in assembler_ppc.inline.hpp as well. I have no further comments :)
>
>
> Best Regards,
> Gustavo
>
> >
> > Best regards,
> > --
> > Michihiro,
> > IBM Research - Tokyo
> >
> > Inactive hide details for Gustavo Romero ---2018/07/25 23:05:32---Hi Michi,
> On 07/25/2018 02:43 AM, Michihiro Horie wrote:Gustavo Romero ---
> 2018/07/25 23:05:32---Hi Michi, On 07/25/2018 02:43 AM, Michihiro Horie
> wrote:
> >
> > From: Gustavo Romero <gromero at linux.vnet.ibm.com>
> > To: Michihiro Horie/Japan/IBM at IBMJP, ppc-aix-port-
> dev at openjdk.java.net, hotspot-dev at openjdk.java.net
> > Cc: goetz.lindenmaier at sap.com, volker.simonis at sap.com, "Doerr, Martin"
> <martin.doerr at sap.com>
> > Date: 2018/07/25 23:05
> > Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
> >
> > -------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> -----------------------------------------------------
> >
> >
> >
> > Hi Michi,
> >
> > On 07/25/2018 02:43 AM, Michihiro Horie wrote:
> > > Dear all,
> > >
> > > Would you review the following change?
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8208171
> > > Webrev: http://cr.openjdk.java.net/~mhorie/8208171/webrev.00
> > >
> > > This change adds support for vectorized arithmetic calculation with SLP.
> > >
> > > The to_vr function is added to convert VSR to VR. Currently, vecX is
> associated with a VSR class vs_reg that only defines VSR32-51 in ppc.ad,
> which are exactly overlapped with VRs. Instruction APIs receiving VRs use the
> to_vr via vecX. Another thing is the change in sqrtF_reg to enable the
> matching with SqrtVF. I think the change in sqrtF_reg would be fine due to
> the ConvD2FNode::Value in convertnode.cpp.
> >
> > Looks good. Just a few comments:
> >
> > - In vmul4F_reg() would it be reasonable to use xvmulsp instead of
> vmaddfp in
> > order to avoid the splat?
> >
> > - Although all instructions added by your change where introduced in ISA
> 2.06,
> > so POWER7 and above are OK, as I see probes for
> PowerArchictecturePPC64=6|5 in
> > vm_version_ppc.cpp (line 64), I'm wondering if there is any control point
> to
> > guarantee that these instructions won't be emitted on a CPU that does
> not
> > support them.
> >
> > - I think that in general string in format %{} are in upper case. For instance,
> > this the current output on optoassembly for vmul4F:
> >
> > 2941835 5b4 ADDI R24, R24, #64
> > 2941836 5b8 vmaddfp VSR32,VSR32,VSR36 ! mul packed4F
> > 2941837 5c0 STXVD2X [R17], VSR32 // store 16-byte Vector
> >
> > I think it would be better to be in upper case instead. I also think that if
> > the node match emits more than one instruction all instructions must be
> listed
> > in format %{}, since it's meant for detailed debugging. Finally I think it
> > would be better to replace \t! by \t// in that string (unless I'm missing any
> > special meaning for that char). So for vmul4F it would be something like:
> >
> > 2941835 5b4 ADDI R24, R24, #64
> > VSPLTISW VSR34, 0 // Splat 0 imm in VSR34
> > 2941836 5b8 VMADDFP VSR32,VSR32,VSR36,VSR34 // Mul packed4F
> > 2941837 5c0 STXVD2X [R17], VSR32 // store 16-byte Vector
> >
> >
> > But feel free to change anything just after you get additional reviews :)
> >
> >
> > > I confirmed this change with JTREG. In addition, I used attached micro
> benchmarks.
> > > /(See attached file: slp_microbench.zip)/
> >
> > Thanks for sharing it.
> > Btw, another option to host it would be in the CR
> > server, in http://cr.openjdk.java.net/~mhorie/8208171
> >
> >
> > Best regards,
> > Gustavo
> >
> > >
> > > Best regards,
> > > --
> > > Michihiro,
> > > IBM Research - Tokyo
> > >
> >
> >
> >
More information about the hotspot-dev
mailing list