RFR: 8319947: Recursive lightweight locking: s390x implementation

[Axel Boldt-Christmas]

On Wed, 22 May 2024 07:12:30 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> 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.

I still do not think I understand what ` if we load top there then it could result into infinite loop` is referring to.

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

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