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