RFR: 8333382: [s390x] Enhance popcnt Instruction to use Z15 facilities

[Lutz Schmidt]

On Sat, 1 Jun 2024 13:15:45 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

> We need to move popcnt instruction implementation out of s390.ad file as it is required to be required some methods present in [JDK-8331126.](https://bugs.openjdk.org/browse/JDK-8331126)
> 
> 
> When the miscellaneous-instruction-extensions facility 3 is not installed or bit 0 of the M3
>  field is zero, a count of the number of one bits in each of the eight bytes of general register
>  R2 is placed into the corresponding byte of general register R1. Each byte of general register 
> R1 is an 8-bit binary integer in the range of 0-8.
> 
> 
> 
> When the miscellaneous-instruction-extensions facility 3 is installed and bit 0 of the M3 field
> is one, a count of the total number of one bits in the 64-bit general register R2 is placed into 
> general register R1. The result is a 64-bit unsigned integer in the range 0 to 64.
> 
> 
> Performed tier1 test on fastdebug build and didn't see any regression.

Changes requested by lucy (Reviewer).

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

> 3093:   // Ppopulation count intrinsics.
> 3094:   inline void z_flogr(Register r1, Register r2);    // find leftmost one
> 3095:   inline void z_popcnt(Register r1, Register r2, int64_t m3 = 0);   // population count

I do not like optional parameters if not urgently required. Why not always pass a mask value (0 or 8)?

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 5811:

> 5809: 
> 5810:   if (VM_Version::has_MiscInstrExt3()) {
> 5811:     z_llgfr(r_src, r_src);

You should not modify the src register. Register allocation might not expect that. Copy into r_dst instead.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 5815:

> 5813:   } else {
> 5814: 
> 5815: #ifdef ASSERT

You don't need this #ifdef if all enclosed code is just assert().

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 5839:

> 5837:   } else {
> 5838: 
> 5839: #ifdef ASSERT

You don't need this #ifdef if all enclosed code is just assert().

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

> 10690: 
> 10691:     // Prefer compile-time assertion over run-time SIGILL.
> 10692:     assert(VM_Version::has_PopCount(), "bad predicate for popCountI_Ext3");

Duplicate check!

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

> 10712: 
> 10713:     // Prefer compile-time assertion over run-time SIGILL.
> 10714:     assert(VM_Version::has_PopCount(), "bad predicate for popCountL_Ext3");

Duplicate check!

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

> 10733: 
> 10734:     // Prefer compile-time assertion over run-time SIGILL.
> 10735:     assert(VM_Version::has_PopCount(), "bad predicate for popCountI");

Duplicate check! But you may want to check for different registers (if you don't trust the effect().

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

> 10755: 
> 10756:     // Prefer compile-time assertion over run-time SIGILL.
> 10757:     assert(VM_Version::has_PopCount(), "bad predicate for popCountL");

Duplicate check! But you may want to check for different registers (if you don't trust the effect().

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

PR Review: https://git.openjdk.org/jdk/pull/19509#pullrequestreview-2104342546
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631083645
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631082265
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631173505
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631173942
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631180515
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631181798
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631182531
PR Review Comment: https://git.openjdk.org/jdk/pull/19509#discussion_r1631190032