RFR: 8327652: S390x: Implements SLP support [v5]
Amit Kumar
amitkumar at openjdk.org
Mon Mar 25 05:32:27 UTC 2024
On Wed, 20 Mar 2024 16:32:50 GMT, Sidraya Jayagond <sjayagond at openjdk.org> wrote:
>> This PR Adds SIMD support on s390x.
>
> Sidraya Jayagond has updated the pull request incrementally with one additional commit since the last revision:
>
> Address code cleanup review comments
I have gone through instruction checking with PofZ book. Only few cosmetic changes are required. I couldn't found anything else.
It would be better @RealLucy could take a look at it, at least "s390.ad" file.
@sid8606 please update copyright header as well. In multiple files, header is not updated.
src/hotspot/cpu/s390/assembler_s390.hpp line 1300:
> 1298: #define VFD_ZOPC (unsigned long)(0xe7L << 40 | 0xE5L << 0) // V1 := V2 / V3, element size = 2**m
> 1299: #define VFSQ_ZOPC (unsigned long)(0xe7L << 40 | 0xCEL << 0) // V1 := sqrt of V2, element size = 2**m
> 1300: #define VFLR_ZOPC (unsigned long)(0xe7L << 40 | 0xC5L << 0) // V1 := sqrt of V2, element size = 2**m
This one is not for square root. Please fix the comment.
src/hotspot/cpu/s390/assembler_s390.hpp line 2498:
> 2496: inline void z_vlef( VectorRegister v1, int64_t d2, Register x2, Register b2, int64_t m3);
> 2497: inline void z_vleg( VectorRegister v1, int64_t d2, Register x2, Register b2, int64_t m3);
> 2498: inline void z_vl(VectorRegister v1, const Address& a);
Suggestion:
inline void z_vl( VectorRegister v1, const Address& a);
src/hotspot/cpu/s390/assembler_s390.hpp line 2628:
> 2626: inline void z_vsteg( VectorRegister v1, int64_t d2, Register x2, Register b2, int64_t m3);
> 2627: inline void z_vstl( VectorRegister v1, Register r3, int64_t d2, Register b2);
> 2628: inline void z_vst(VectorRegister v1, const Address& a);
Suggestion:
inline void z_vst( VectorRegister v1, const Address& a);
src/hotspot/cpu/s390/assembler_s390.hpp line 2687:
> 2685: inline void z_vmlb( VectorRegister v1, VectorRegister v2, VectorRegister v3);
> 2686: inline void z_vmlhw( VectorRegister v1, VectorRegister v2, VectorRegister v3);
> 2687: inline void z_vmlf( VectorRegister v1, VectorRegister v2, VectorRegister v3);
Suggestion:
inline void z_vmlb( VectorRegister v1, VectorRegister v2, VectorRegister v3);
inline void z_vmlhw( VectorRegister v1, VectorRegister v2, VectorRegister v3);
inline void z_vmlf( VectorRegister v1, VectorRegister v2, VectorRegister v3);
src/hotspot/cpu/s390/assembler_s390.hpp line 2824:
> 2822: inline void z_vpopcth( VectorRegister v1, VectorRegister v2);
> 2823: inline void z_vpopctf( VectorRegister v1, VectorRegister v2);
> 2824: inline void z_vpopctg( VectorRegister v1, VectorRegister v2);
Suggestion:
inline void z_vpopctb(VectorRegister v1, VectorRegister v2);
inline void z_vpopcth(VectorRegister v1, VectorRegister v2);
inline void z_vpopctf(VectorRegister v1, VectorRegister v2);
inline void z_vpopctg(VectorRegister v1, VectorRegister v2);
src/hotspot/cpu/s390/assembler_s390.hpp line 2916:
> 2914: // ==========================
> 2915: // Add
> 2916: inline void z_vfa(VectorRegister v1, VectorRegister v2, VectorRegister v3, int64_t m4);
Suggestion:
inline void z_vfa( VectorRegister v1, VectorRegister v2, VectorRegister v3, int64_t m4);
src/hotspot/cpu/s390/assembler_s390.hpp line 2921:
> 2919:
> 2920: //SUB
> 2921: inline void z_vfs(VectorRegister v1, VectorRegister v2, VectorRegister v3, int64_t m4);
Suggestion:
inline void z_vfs( VectorRegister v1, VectorRegister v2, VectorRegister v3, int64_t m4);
src/hotspot/cpu/s390/assembler_s390.hpp line 2931:
> 2929:
> 2930: //DIV
> 2931: inline void z_vfd(VectorRegister v1, VectorRegister v2, VectorRegister v3, int64_t m4);
Suggestion:
inline void z_vfd( VectorRegister v1, VectorRegister v2, VectorRegister v3, int64_t m4);
src/hotspot/cpu/s390/assembler_s390.hpp line 2936:
> 2934:
> 2935: //square root
> 2936: inline void z_vfsq(VectorRegister v1, VectorRegister v2, int64_t m3);
Suggestion:
inline void z_vfsq( VectorRegister v1, VectorRegister v2, int64_t m3);
src/hotspot/cpu/s390/assembler_s390.hpp line 2941:
> 2939:
> 2940: //vector fp load rounded
> 2941: inline void z_vflr( VectorRegister v1, VectorRegister v2, int64_t m3, int64_t m5);
Suggestion:
inline void z_vflr( VectorRegister v1, VectorRegister v2, int64_t m3, int64_t m5);
src/hotspot/cpu/s390/assembler_s390.hpp line 2944:
> 2942: inline void z_vflrd( VectorRegister v1, VectorRegister v2, int64_t m5);
> 2943:
> 2944: // Vector Floatingpoint instructions
No, these instructions seems simple floating point instructions, not vector floating point.
src/hotspot/cpu/s390/assembler_s390.inline.hpp line 1207:
> 1205: inline void Assembler::z_vfssb( VectorRegister v1, VectorRegister v2, VectorRegister v3) {z_vfs(v1, v2, v3, VRET_FW); } // vector element type 'F'
> 1206: inline void Assembler::z_vfsdb( VectorRegister v1, VectorRegister v2, VectorRegister v3) {z_vfs(v1, v2, v3, VRET_DW); } // vector element type 'G'
> 1207: //
Suggestion:
src/hotspot/cpu/s390/assembler_s390.inline.hpp line 1223:
> 1221: inline void Assembler::z_vfsqsb( VectorRegister v1, VectorRegister v2) {z_vfsq(v1, v2, VRET_FW); }
> 1222: inline void Assembler::z_vfsqdb( VectorRegister v1, VectorRegister v2) {z_vfsq(v1, v2, VRET_DW); }
> 1223:
maybe add commend regarding Rounding Instructions ?
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 264:
> 262:
> 263: static const RegisterSaver::LiveRegType RegisterSaver_LiveVRegs[] = {
> 264: // live vector registers (optional, only these ones are used by C2):
Suggestion:
// live vector registers (optional, only these are used by C2):
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 296:
> 294: }
> 295:
> 296:
Suggestion:
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 422:
> 420: map->set_callee_saved(VMRegImpl::stack2reg(offset>>2), RegisterSaver_LiveVRegs[i].vmreg);
> 421: offset += v_reg_size;
> 422: }
just in case you like this one.
Suggestion:
for (int i = 0; i < vregstosave_num; i++, offset += v_reg_size) {
int reg_num = RegisterSaver_LiveVRegs[i].reg_num;
__ z_vst(as_VectorRegister(reg_num), Address(Z_SP, offset));
map->set_callee_saved(VMRegImpl::stack2reg(offset>>2), RegisterSaver_LiveVRegs[i].vmreg);
}
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 576:
> 574:
> 575: offset += v_reg_size;
> 576: }
push if you like ;-)
Suggestion:
for (int i = 0; i < vregstosave_num; i++, offset += v_reg_size) {
int reg_num = RegisterSaver_LiveVRegs[i].reg_num;
__ z_vl(as_VectorRegister(reg_num), Address(Z_SP, offset));
}
-------------
Changes requested by amitkumar (Committer).
PR Review: https://git.openjdk.org/jdk/pull/18162#pullrequestreview-1953948697
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537026170
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537020685
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537021563
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537022342
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537023129
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537025417
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537025529
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537025651
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537025795
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537025868
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537024327
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537034274
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537040800
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1535054098
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1535056022
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1535068418
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1535071035
More information about the hotspot-dev
mailing list