On Wed, 22 Mar 2023 00:25:43 GMT, Vladimir Kozlov <k...@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 670: > >> 668: get_thread (scrReg); // beware: clobbers ICCs >> 669: movptr(Address(boxReg, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), >> scrReg); >> 670: xorptr(boxReg, boxReg); // set icc.ZFlag = 1 to >> indicate success > > Should this be under `if (UseFastLocking)`? I don't think so, unless we also want to change all the stuff in x86_32.ad to not fetch the thread before calling into fast_unlock(). However, I think it is a nice and useful change. I could break it out of this PR and get it reviewed separately, it is a side-effect of the new locking impl insofar as we always require a thread register, and allocate&fetch it before going into fast_lock(). > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 791: > >> 789: Compile::current()->output()->add_stub(stub); >> 790: jcc(Assembler::notEqual, stub->entry()); >> 791: bind(stub->continuation()); > > Why use stub here and not inline the code? Because the branch mostly not > taken? Yes, the branch is mostly not taken. If we inline the code, then we would have to take a forward branch on the very common path to skip over the (rare) part that handles ANON monitor owner. This would throw off static branch prediction and is discouraged by the Intel optimization guide. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1144501909 PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1144504528
