RFR: 8349686: [s390x] C1: Improve Class.isInstance intrinsic [v4]

[Lutz Schmidt]

On Tue, 11 Feb 2025 08:47:24 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> s390x implementation for Class.isInstance intrinsic. 
>> 
>> Tier1 test on release & fastdebug vm are clean with flag: `-XX:-UseSecondarySupersCache -XX:+UseSecondarySupersTable -XX:+VerifySecondarySupers -XX:+StressSecondarySupers`. 
>> 
>> Benchmark results will be updated soon.
>
> Amit Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update comment

Looks good overall. Some details need to be addressed.

src/hotspot/cpu/s390/c1_Runtime1_s390.cpp line 639:

> 637:       __ push_frame(frame_size);
> 638: 
> 639:       // Z_R10 and Z_R11 are call saved, so we must push them before any use

pls write **caller** saved.

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

> 3672:                                                    Register r_temp2,
> 3673:                                                    Register r_temp3) {
> 3674:   assert_different_registers(r_sub_klass, r_super_klass, r_result, r_temp1, r_temp2, r_temp3, Z_R0_scratch);

Better use `LOCGHI` further down and avoid use of `Z_R0_scratch`. 
You are using `LOCHI` in `Runtime1::generate_code_for()` anyway which implies you are sure the load/store-on-condition facility 2 is installed. In other words: is z13 the minimum H/W version?

Even more simplification: there is no need to set `r_linear_result` conditionally. You set it to 1 and branch to failure if array length is zero. For all other cases, repne_scan() does the right thing.

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

> 3718:   z_lg(Z_ARG2, -16, Z_SP); // r_sub_klass
> 3719:   z_lg(Z_ARG3, -24, Z_SP); // r_linear_result
> 3720: 

This argument shuffle implementation works well in 99.99% of all situations. Maybe it's even more reliable. 

BUT: you are using stack space which is outside the bounds of used (and thus protected) stack space. If your thread is catching an interrupt (a signal, for example), the interrupt handler will place its data just beyond Z_SP. 

You MUST resize the frame to make room for the needed spill space.

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

Changes requested by lucy (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23535#pullrequestreview-2609555124
PR Review Comment: https://git.openjdk.org/jdk/pull/23535#discussion_r1951378397
PR Review Comment: https://git.openjdk.org/jdk/pull/23535#discussion_r1951320369
PR Review Comment: https://git.openjdk.org/jdk/pull/23535#discussion_r1951360524