RFR: 8308479: [s390x] Implement alternative fast-locking scheme [v3]

[Lutz Schmidt]

On Tue, 13 Jun 2023 12:51:15 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> This PR implements new fast-locking scheme for s390x. Additionally few parameters have been renamed to be in sync with PPC.
>> 
>> Testing done (for release, fastdebug and slowdebug build):
>> All `test/jdk/java/util/concurrent` test with parameters:
>> * LockingMode=2 
>> * LockingMode=2 with -Xint
>> * LockingMode=2 with -XX:TieredStopAtLevel=1
>> * LockingMode=2 with -XX:-TieredCompilation
>> 
>> Result is consistently similar to Aarch(MacOS) and PPC, All of 124 tests are passing except `MapLoops.java` because in the 2nd part for this testcase, jvm starts with `HeavyMonitors` which conflict with `LockingMode=2`
>
> Amit Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   restore currentHeader after z_nill

There are some comments you may want to consider.

src/hotspot/cpu/s390/c1_MacroAssembler_s390.hpp line 54:

> 52:   // obj     : Must point to the object to lock, contents preserved.
> 53:   // disp_hdr: Must point to the displaced header location, contents destroyed.
> 54:   // Z_R0_scratch will be killed as well

Shouldn't that read 
`// Z_R1_scratch will be killed as well`
Such comment should be added to lock_object as well.
How can `void lock_object()` return anything?

src/hotspot/cpu/s390/c1_MacroAssembler_s390.hpp line 55:

> 53:   // disp_hdr: Must point to the displaced header location, contents destroyed.
> 54:   // Z_R0_scratch will be killed as well
> 55:   void unlock_object(Register hdr, Register obj, Register lock, Label& slow_case);

Please adjust parameter names to what is used in the *.cpp file.
This applies to lock_object as well.

src/hotspot/cpu/s390/interp_masm_s390.cpp line 1176:

> 1174:     z_afi(tmp, -oopSize);
> 1175:     z_lg(tmp, Address(Z_thread, tmp));
> 1176:     z_cgrj(tmp, object, Assembler::bcondNotEqual, slow_case);

You should decide how to treat the value in tmp:

- is it 32-bit or 64-bit?
- is it signed or unsigned?

After you made up your mind, use the instructions that match your decision.

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

> 5668: 
> 5669:   // Clear lock-bits from hdr (locked state)
> 5670:   z_xilf(temp, markWord::unlocked_value);

The markWord values occupy only the rightmost few bits. Therefore, z_xill() will suffice.

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

> 5676:   z_lgf(temp, Address(Z_thread, JavaThread::lock_stack_top_offset()));
> 5677:   z_stg(obj, Address(Z_thread, temp));
> 5678:   z_afi(temp, oopSize);

oopSize is small, z_ahi() will suffice.

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

> 5681:   // as locking was successful, set CC to EQ
> 5682:   z_lghi(temp, 0);
> 5683:   z_ltgr(temp, temp);

How about using
`  z_cr(temp, temp);`
instead? Achieves the same thing with one instruction (2 bytes) instead of two instructions (8 bytes).

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

> 5746:   z_st(tmp, Address(Z_thread, JavaThread::lock_stack_top_offset()));
> 5747:   z_ltgr(zero, zero); // set CC to EQ
> 5748: }

To save a few bytes and a tick or two:

  // After successful unlock, pop object from lock-stack
  z_lgf(tmp, Address(Z_thread, JavaThread::lock_stack_top_offset()));
  z_afi(tmp, -oopSize);
#ifdef ASSERT
  z_lghi(zero, 0); // Z_R0_scratch
  z_stg(zero, Address(Z_thread, tmp));
#endif
  z_st(tmp, Address(Z_thread, JavaThread::lock_stack_top_offset()));
  z_cr(tmp, tmp); // set CC to EQ

or even more sophisticated (no need for second scratch register and just 8 bytes of code with ASSERT undefined)

  // After successful unlock, pop object from lock-stack
#ifdef ASSERT
  z_lgf(tmp, Address(Z_thread, JavaThread::lock_stack_top_offset()));
  z_aghi(tmp, -oopSize);
  z_agr(tmp, Z_thread);
  z_xc(0, oopSize-1, tmp, 0, tmp);  // wipe out lock-stack entry
#endif
  z_alsi(JavaThread::lock_stack_top_offset(), Z_thread, -oopSize);  // pop object
  z_cr(tmp, tmp); // set CC to EQ

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

Changes requested by lucy (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14414#pullrequestreview-1479261327
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229497227
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229575356
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229607260
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229698866
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229683720
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229634411
PR Review Comment: https://git.openjdk.org/jdk/pull/14414#discussion_r1229650949