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

[Lutz Schmidt]

On Mon, 25 Mar 2024 12:39:35 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:
> 
>   Fix cosmetic review comments

Pretty well done! 
I have added a few comments/suggestions. 
Important:
This implementation breaks the CompactStrings intrinsics:

MacroAssembler::string_compress(...)
MacroAssembler::string_expand(...)

If available, these intrinsics use vector registers and vector instructions. The registers are just used, without requesting them from the register allocator. A simple remedy would be:

- resize (expand) the current stack frame
- save the used vector registers in the additional stack space
- run the vector code
- restore the used vector registers
- restore the original stack frame size

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

> 1192:     }
> 1193:     return size;
> 1194:   }

How about doing the size calculation like 

  if (cbuf) {
    C2_MacroAssembler _masm(cbuf);
    int start_off = __ offset();
    __ z_vlr(Rdst, Rsrc);
    size += __offset() - start_off;
  }

That way, you are sure to have the correct size. And you only increase the size if you actually emit code.

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

> 11036:   %}
> 11037:   ins_pipe(pipe_class_dummy);
> 11038: %}

Move this instruct up to after vadd4I_reg, please.

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

> 11038: %}
> 11039: 
> 11040: instruct vsub416B_reg(vecX dst, vecX src1, vecX src2) %{

vsub416B_reg? Typo?

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

> 313:   const int vregstosave_num     = save_vectors ? (sizeof(RegisterSaver_LiveVRegs) /
> 314:                                                   sizeof(RegisterSaver::LiveRegType))
> 315:                                                   : 0;

I'd like to have this calculation in only one place. Don't duplicate code, in particular when it's a calculation that must yield the same result.

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

> 486:   const int vregstosave_num     = save_vectors ? (sizeof(RegisterSaver_LiveVRegs) /
> 487:                                                   sizeof(RegisterSaver::LiveRegType))
> 488:                                                   : 0;

see above (save_live_registers())

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

Changes requested by lucy (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18162#pullrequestreview-1958239716
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537933537
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1539441724
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1539446392
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537893144
PR Review Comment: https://git.openjdk.org/jdk/pull/18162#discussion_r1537902448