RFR: 8313438: [s390x] build broken after JDK-8301996 [v2]

[Martin Doerr]

On Thu, 5 Oct 2023 10:18:11 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> Build Fix after JDK-8301996. 
>> 
>> Warnings related to new Lightweight locking scheme are seen during the build phase, but we will get rid of them, after integrating #14414 . Testing done for `release-vm`, `slowdebug-vm`, `fastdebug-vm` and `optimized-vm`.
>> 
>> 3 Test failures seen on s390x are un-related to these changes.
>> 
>> jdk/jshell/ClassMembersTest.java
>> java/lang/invoke/condy/CondyWrongType.java
>> java/lang/invoke/condy/CondyWithGarbageTest.java
>> 
>> JBS issue for test failures: [JDK-8317581](https://bugs.openjdk.org/browse/JDK-8317581).
>
> Amit Kumar has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
> 
>   s390 Invoke Field Port

Looks basically good, but I have a couple of questions and suggestions.

src/hotspot/cpu/s390/templateTable_s390.cpp line 82:

> 80:     __ z_larl(br_tab_temp, (int64_t)0);  /* Check current address alignment. */\
> 81:     __ z_slgr(br_tab_temp, br_tab);      /* Current Address must be equal    */\
> 82:     if (tos_state != Z_R0_scratch) {                                           \

Where can tos_state be R0?

src/hotspot/cpu/s390/templateTable_s390.cpp line 86:

> 84:     }                                                                          \
> 85:     __ z_brc(Assembler::bcondLogZero, 4);/* skip trap if ok. */                \
> 86:     __ z_illtrap(0x55);                                                        \

Is z_illtrap duplicated by intention? If so, please explain in a comment.

src/hotspot/cpu/s390/templateTable_s390.cpp line 2453:

> 2451: 
> 2452:   // TOS state
> 2453:   __ load_sized_value(tos_state, Address(cache, in_bytes(ResolvedFieldEntry::type_offset())), sizeof(u1), false);

Note that tos_state is not needed by all usages. Making this optional saves an instruction and a register (see PPC64 implementation).

src/hotspot/cpu/s390/templateTable_s390.cpp line 3006:

> 3004:   // Displacement is 0. No need to care about limited displacement range.
> 3005:   const Address field(fieldAddr);
> 3006:   __ lgr_if_needed(fieldAddr, off);

Was there a problem with the range? Why did it work in the old implementation?

src/hotspot/cpu/s390/templateTable_s390.cpp line 3032:

> 3030: 
> 3031: #ifdef ASSERT
> 3032:   const unsigned int bsize = is_static ? BTB_MINSIZE*1 : BTB_MINSIZE*8; // TODO: why code size is increasing ?

TODO should get resolved.

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

PR Review: https://git.openjdk.org/jdk/pull/15885#pullrequestreview-1661523602
PR Review Comment: https://git.openjdk.org/jdk/pull/15885#discussion_r1348517775
PR Review Comment: https://git.openjdk.org/jdk/pull/15885#discussion_r1348509080
PR Review Comment: https://git.openjdk.org/jdk/pull/15885#discussion_r1348524126
PR Review Comment: https://git.openjdk.org/jdk/pull/15885#discussion_r1348545312
PR Review Comment: https://git.openjdk.org/jdk/pull/15885#discussion_r1348545858