RFR: 8208171: PPC64: Enrich SLP support
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Sep 4 06:12:19 UTC 2018
> > Why do you rename vnoreg to vnoregi?
> I followed the way of coding for vsnoreg and vsnoregi, but the renaming
> does not look necessary. I would get this part back. Should I also rename
> vsnoregi to vsnoreg?
I think it would be more consistent, but it's not that important :)
Best regards,
Goetz.
>
>
> >we noticed jtreg test failures when using this change:
> >compiler/runtime/safepoints/TestRegisterRestoring.java
> >compiler/runtime/Test7196199.java
> >
> >TestRegisterRestoring is a simple test which returns arbitrary results instead
> of 10000.
> >
> >We didn't see it on all machines, so it might be an issue with
> saving&restoring VR registers in the signal handler.
> >The machine which I have used has "SUSE Linux Enterprise Server 12 SP3"
> with kernel 4.4.126-94.22-default.
> Thank you for letting me know the issue, I will try to reproduce this on a SUSE
> machine.
>
>
> >I also noticed that "-XX:-SuperwordUseVSX" crashes with bad ad file when
> your patch is applied. Looks like matching the vector nodes needs to be
> prevented.
> Thank you for pointing out another issue. Currently I do not hit this problem,
> but preventing to match the vector nodes makes sense to avoid the crash. I
> did not prepare match rules for non-vector nodes, so it might be better to
> prepare them similarly like the Replicate* rules, in any case.
>
>
> Gustavo, thanks for the wrap-up!
>
>
> Best regards,
> --
> Michihiro,
> IBM Research - Tokyo
>
> "Doerr, Martin" ---2018/09/04 02:18:24---Hi Gustavo and Michihiro, we
> noticed jtreg test failures when using this change:
>
> From: "Doerr, Martin" <martin.doerr at sap.com>
> To: Gustavo Romero <gromero at linux.vnet.ibm.com>, "Lindenmaier, Goetz"
> <goetz.lindenmaier at sap.com>, Michihiro Horie <HORIE at jp.ibm.com>
> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>, "hotspot-
> dev at openjdk.java.net" <hotspot-dev at openjdk.java.net>
> Date: 2018/09/04 02:18
> Subject: RE: RFR: 8208171: PPC64: Enrich SLP support
>
> ________________________________
>
>
>
>
> Hi Gustavo and Michihiro,
>
> we noticed jtreg test failures when using this change:
> compiler/runtime/safepoints/TestRegisterRestoring.java
> compiler/runtime/Test7196199.java
>
> TestRegisterRestoring is a simple test which returns arbitrary results instead
> of 10000.
>
> We didn't see it on all machines, so it might be an issue with saving&restoring
> VR registers in the signal handler.
> The machine which I have used has "SUSE Linux Enterprise Server 12 SP3"
> with kernel 4.4.126-94.22-default.
>
> That's what I found out so far. Maybe you have an idea?
>
> I also noticed that "-XX:-SuperwordUseVSX" crashes with bad ad file when
> your patch is applied. Looks like matching the vector nodes needs to be
> prevented.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
> Gustavo Romero
> Sent: Montag, 3. September 2018 14:57
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Michihiro Horie
> <HORIE at jp.ibm.com>
> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>; hotspot-
> dev at openjdk.java.net
> Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
>
> Hi Goetz,
>
> On 09/03/2018 09:27 AM, Lindenmaier, Goetz wrote:
> > 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/ .
>
> Yes, probably it was a problem of the pipermail or in some relay.
> I noted the same thing, i.e. at least one Michi reply arrived
> to me but missed a ML.
>
> The initial discussion is here:
> http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-
> July/003613.html
>
> I understand Martin reviewed the last webrev in that thread, which is
> http://cr.openjdk.java.net/~mhorie/8208171/webrev.01/ (taken from
> http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-
> July/003615.html)
>
> Martin's review of webrev.01:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-
> August/033958.html
>
> and Michi's reply to Martin's review of webrev.01:
> http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-
> August/003632.html (with webrev.02,
> taken from http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-
> August/003632.html).
>
> and your last review:
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-
> September/030419.html
>
>
> HTH.
>
> Best regards,
> Gustavo
>
> > 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-compiler-dev
mailing list