[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    RE: RFR: 8208171: PPC64: Enrich SLP support
From:       "Doerr, Martin" <martin.doerr () sap ! com>
Date:       2018-08-28 17:34:44
Message-ID: 346da54af45243c4bdaf475f118a450d () sap ! com
[Download RAW message or body]

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@linux.vnet.ibm.com> 
Sent: Donnerstag, 26. Juli 2018 16:02
To: Michihiro Horie <HORIE@jp.ibm.com>
Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-dev@openjdk.java.net; \
Doerr, Martin <martin.doerr@sap.com>; ppc-aix-port-dev@openjdk.java.net; Simonis, \
                Volker <volker.simonis@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@linux.vnet.ibm.com>
> To: Michihiro Horie/Japan/IBM@IBMJP, ppc-aix-port-dev@openjdk.java.net, \
>                 hotspot-dev@openjdk.java.net
> Cc: goetz.lindenmaier@sap.com, volker.simonis@sap.com, "Doerr, Martin" \
>                 <martin.doerr@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
> > 
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]