RFR: 8308469: [PPC64] Implement alternative fast-locking scheme [v2]

[Martin Doerr]

On Thu, 25 May 2023 13:52:34 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

>> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   interp_masm: Rename displaced_header to header.
>
> src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp line 1101:
> 
>> 1099:       // must handle it.
>> 1100:       Register tmp = current_header;
>> 1101:       // First check for lock-stack underflow.
> 
> I wonder why this is not done on x86? It's better to have that check.

Strange. The other platforms have it.

> src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp line 1111:
> 
>> 1109:       bne(CCR0, slow_case);
>> 1110: 
>> 1111:       ld(displaced_header, oopDesc::mark_offset_in_bytes(), object);
> 
> Maybe rename `displaced_header` to just `header`?

Good idea. Done.

> src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 2854:
> 
>> 2852:     andi_(R0, temp, ObjectMonitor::ANONYMOUS_OWNER);
>> 2853:     bne(CCR0, anonymous);
>> 2854:   }
> 
> This is surly very rare. Can't we take the slow path, i.e. branch to `failure`?

I had thought that, too. Yes, it is possible. But then I thought they probably added the path for C2 for a reason. Otherwise they would probably not have spent the extra effort for implementing the stub. So, I prefer to have it, too.

> src/hotspot/cpu/ppc/ppc.ad line 12142:
> 
>> 12140: // inlined locking and unlocking
>> 12141: 
>> 12142: instruct cmpFastLock(flagsRegCR0 crx, iRegPdst oop, iRegPdst box, iRegPdst tmp1, iRegPdst tmp2) %{
> 
> I'm a little concerned about the usage of CR0. First it's not always clear if CR0 is altered when using the MacroAssembler, especially if not hacking ppc assembler on a daily basis. And also there might be a few instruction forms in the AD files that haven't got an effect defined for CR0 even though they alter it.

There are already some nodes which use CR0 as result which is tested for quite some time. Nodes which kill it should ideally have a "kill cr0" effect, but we don't need to rely on it. C2 doesn't place other nodes between the one which sets the ConditionRegister and the conditional branch. So, using CR0 should be reliable.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14069#discussion_r1205792963
PR Review Comment: https://git.openjdk.org/jdk/pull/14069#discussion_r1205793141
PR Review Comment: https://git.openjdk.org/jdk/pull/14069#discussion_r1205795880
PR Review Comment: https://git.openjdk.org/jdk/pull/14069#discussion_r1205792643