On Fri, 24 Mar 2023 11:17:05 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Roman Kennke has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/JDK-8291555-v2' into JDK-8291555-v2 >> - Set condition flags correctly after fast-lock call on aarch64 > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 611: > >> 609: bind(slow); >> 610: testptr(objReg, objReg); // force ZF=0 to indicate failure >> 611: jmp(NO_COUNT); > > We set a flag on failure (`NO_COUNT`) path. Should we set the flag on success > (`COUNT`) path as well? The path at COUNT already sets the ZF, there is no need to do it here. NO_COUNT doesn't clear ZF, and fast_lock_impl() may branch to slow with ZF set (on the overflow check) so we need to explicitly clear ZF. However, I just came up with a better way to check for overflow that readily clears the ZF: subtract 1 from the end-offset and make a greater-comparison instead of the greaterEquals that we currently do on the end-offset. That should simplify the code and avoid a branch. > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 926: > >> 924: mov(boxReg, tmpReg); >> 925: fast_unlock_impl(objReg, boxReg, tmpReg, NO_COUNT); >> 926: jmp(COUNT); > > Do we need to care about returning proper flags here? Yes we do, but fast_unlock_impl() already does the right thing on the failure path, and COUNT does the right thing on the success path. Yes, it is all very ugly. *shrugs* ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1150609171 PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1150610895
