RFR: 8319947: Recursive lightweight locking: s390x implementation

[Axel Boldt-Christmas]

On Tue, 21 May 2024 12:55:09 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> src/hotspot/cpu/s390/macroAssembler_s390.cpp line 6015:
>> 
>>> 6013:     // we will encounter a loop while handling the inflated monitor case
>>> 6014:     // so, we need to make sure, when we reach there only top one object is removed.
>>> 6015:     // if we load top there then it could result into infinite loop, So preserving top is a Must here;
>> 
>> I assume this refers to the assert/debug code that checks that the lock stack does not contain the object when doing inflated unlocking. The debug code could just unconditionally reload top from the thread.
>
> probably we can do something like this: 
> 
> diff --git a/src/hotspot/cpu/s390/macroAssembler_s390.cpp b/src/hotspot/cpu/s390/macroAssembler_s390.cpp
> index 5a77e0d49f2..cfde21c84f0 100644
> --- a/src/hotspot/cpu/s390/macroAssembler_s390.cpp
> +++ b/src/hotspot/cpu/s390/macroAssembler_s390.cpp
> @@ -5977,7 +5977,7 @@ void MacroAssembler::compiler_fast_unlock_lightweight_object(Register obj, Regis
>    assert_different_registers(obj, tmp1, tmp2);
>  
>    // Handle inflated monitor.
> -  NearLabel inflated, inflated_load_monitor;
> +  NearLabel inflated, inflated_load_monitor, inflated_intermediate ;
>    // Finish fast unlock successfully. MUST reach to with flag == EQ.
>    NearLabel unlocked;
>    // Finish fast unlock unsuccessfully. MUST branch to with flag == NE.
> @@ -6021,7 +6021,7 @@ void MacroAssembler::compiler_fast_unlock_lightweight_object(Register obj, Regis
>      // Check for monitor (0b10).
>      z_lg(mark, Address(obj, oopDesc::mark_offset_in_bytes()));
>      z_tmll(mark, markWord::monitor_value);
> -    z_brnaz(inflated);
> +    z_brnaz(inflated_intermediate);
>  
>  #ifdef ASSERT
>      // Check header not unlocked (0b01).
> @@ -6063,6 +6063,8 @@ void MacroAssembler::compiler_fast_unlock_lightweight_object(Register obj, Regis
>      stop("Fast Unlock not monitor");
>  #endif // ASSERT
>  
> +    bind(inflated_intermediate);
> +    z_lgf(top, Address(Z_thread, JavaThread::lock_stack_top_offset()));
>      bind(inflated);
>  
>  #ifdef ASSERT
> 
> 
> But instead I kept the code a bit similar to other architectures and for future just added the comment as a warning to be careful which tweaking the code. I guess except this comment everything is fine ?

The current code is fine, but that comment made me wonder why preserving the original top value was important. My think was that you could only change the assert snippet as follows:

#ifdef ASSERT
    NearLabel check_done;
+   NearLabel loop;
+   z_lgf(top, Address(Z_thread, JavaThread::lock_stack_top_offset()));
+   bind(loop);
    z_aghi(top, -oopSize);
    compareU32_and_branch(top, in_bytes(JavaThread::lock_stack_base_offset()),
                          bcondLow, check_done);
    z_cg(obj, Address(Z_thread, top));
-   z_brne(inflated);
+   z_brne(loop);
    stop("Fast Unlock lock on stack");
    bind(check_done);
#endif // ASSERT


then remove the comment and use either whatever register.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18878#discussion_r1609416066