Thanks a lot, Martin! Best regards, -- Michihiro, IBM Research - Tokyo From: "Doerr, Martin" <martin.doerr@sap.com> To: Michihiro Horie <HORIE@jp.ibm.com> Cc: "Lindenmaier, Goetz" <goetz.lindenmaier@sap.com>, Gustavo Romero <gromero@linux.vnet.ibm.com>, "hotspot-dev@openjdk.java.net" <hotspot-dev@openjdk.java.net>, "ppc-aix-port-dev@openjdk.java.net" <ppc-aix-port-dev@openjdk.java.net>, "Simonis, Volker" <volker.simonis@sap.com> Date: 2018/09/01 00:28 Subject: RE: RFR: 8208171: PPC64: Enrich SLP support Hi Michihiro, thanks for the update. Looks correct, now. I can also sponsor this change. Does anybody else want to review it? Best regards, Martin From: Michihiro Horie <HORIE@jp.ibm.com> Sent: Freitag, 31. August 2018 15:16 To: Doerr, Martin <martin.doerr@sap.com> Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Gustavo Romero <gromero@linux.vnet.ibm.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net; Simonis, Volker <volker.simonis@sap.com> Subject: RE: RFR: 8208171: PPC64: Enrich SLP support Hi Martin, Thank you so much for giving comments! I fixed version checks and indentation. New webrev: http://cr.openjdk.java.net/~mhorie/8208171/webrev.02/ Best regards, -- Michihiro, IBM Research - Tokyo Inactive hide details for "Doerr, Martin" ---2018/08/29 02:35:05---Hi Michihiro, thank you for implementing it. I have just tak"Doerr, Martin" ---2018/08/29 02:35:05---Hi Michihiro, thank you for implementing it. I have just taken a first look at your webrev.01. From: "Doerr, Martin" <martin.doerr@sap.com> To: Gustavo Romero <gromero@linux.vnet.ibm.com>, Michihiro Horie < HORIE@jp.ibm.com> Cc: "Lindenmaier, Goetz" <goetz.lindenmaier@sap.com>, " hotspot-dev@openjdk.java.net" <hotspot-dev@openjdk.java.net>, " ppc-aix-port-dev@openjdk.java.net" <ppc-aix-port-dev@openjdk.java.net>, "Simonis, Volker" <volker.simonis@sap.com> Date: 2018/08/29 02:35 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@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
