RFR: 8327652: S390x: Implements SLP support [v4]

[Martin Doerr]

On Tue, 19 Mar 2024 13:38:33 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:
> 
>   Use Op_VecX instead of Op_RegF
>   
>   Signed-off-by: Sidraya <sidraya.jayagond at ibm.com>

This looks much better! I have some minor cleanup suggestions. I didn't review the s390 instructions and their usage. That should be done by somebody else, maybe @offamitkumar?

src/hotspot/cpu/s390/assembler_s390.hpp line 2536:

> 2534:   inline void z_vlvgh( VectorRegister v1, Register r3, int64_t d2, Register b2=Z_R0);
> 2535:   inline void z_vlvgf( VectorRegister v1, Register r3, int64_t d2, Register b2=Z_R0);
> 2536:   inline void z_vlvgg( VectorRegister v1, Register r3, int64_t d2, Register b2=Z_R0);

Common coding style: `b2 = Z_R0` with spaces.

src/hotspot/cpu/s390/globals_s390.hpp line 114:

> 112:   product(bool, SuperwordUseVX, false,                                        \
> 113:           "Use Z15 Vector instructions for superword optimization.")          \
> 114:   product(bool, UseSFPV, false, DIAGNOSTIC,                                               \

Please adapt the whitespaces before ``.

src/hotspot/cpu/s390/s390.ad line 1081:

> 1079:   }
> 1080: 
> 1081:   // we have 32 vector register * 4 halves

Seems like this comment needs an update.

src/hotspot/cpu/s390/s390.ad line 10957:

> 10955:   ins_encode( /*empty*/ );
> 10956:   ins_pipe(pipe_class_dummy);
> 10957: %}

Better add an empty line.

src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 314:

> 312: 
> 313:   // Calculate frame size.
> 314:     const int vregstosave_num     = save_vectors ? (sizeof(RegisterSaver_LiveVRegs) /

Indentation.

src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 487:

> 485: void RegisterSaver::restore_live_registers(MacroAssembler* masm, RegisterSet reg_set, bool save_vectors) {
> 486:   int offset;
> 487:   //const int register_save_offset = live_reg_frame_size(reg_set) - live_reg_save_size(reg_set);

Do we need to keep this commented code?

-------------

PR Review: https://git.openjdk.org/jdk/pull/18162#pullrequestreview-1946565035
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1530585064
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1530584154
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1530587847
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1530589851
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1530590990
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1530592534