RFR: 8208171: PPC64: Enrich SLP support

[Michihiro Horie]

Hi Gustavo,

Thank you very much for your helpful comments!

I updated webrev:
http://cr.openjdk.java.net/~mhorie/8208171/webrev.01/


Best regards,
--
Michihiro,
IBM Research - Tokyo



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
>


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20180726/10bac6e4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20180726/10bac6e4/graycol.gif>