On Thu, 18 Dec 2025 08:26:03 GMT, Marc Chevalier <[email protected]> wrote:
> Let's proceed piece by piece. > > # `G1BarrierSetAssembler::g1_write_barrier_pre` in > `g1BarrierSetAssembler_aarch64.cpp` > > https://github.com/openjdk/valhalla/blob/1077e4f9f06336af30d95fc28cbab4d31c9f2a44/src/hotspot/cpu/aarch64/gc/g1/g1BarrierSetAssembler_aarch64.cpp#L216-L220 > > `push_call_clobbered_registers`/`pop_call_clobbered_registers` should be > enough around a runtime call, that's what clobbered registers are. > > But let's dig a bit, that will be useful later! > > > push_call_clobbered_registers() > => push_call_clobbered_registers_except(exclude = empty set) > => push(call_clobbered_gp_registers() - exclude, sp) // with exclude = empty > set > > So, we save at least `call_clobbered_gp_registers` which is defined as: > > https://github.com/openjdk/valhalla/blob/1077e4f9f06336af30d95fc28cbab4d31c9f2a44/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L3614-L3620 > > So, we save r0-r7 and r10-r17 > > # `CardTableBarrierSetAssembler::oop_store_at` in > `cardTableBarrierSetAssembler_aarch64.cpp` > > https://github.com/openjdk/valhalla/blob/1077e4f9f06336af30d95fc28cbab4d31c9f2a44/src/hotspot/cpu/aarch64/gc/shared/cardTableBarrierSetAssembler_aarch64.cpp#L116-L124 > > Digging the history looks like it still makes sense, it doesn't look like an > accident. If we remove the if-branch, tests are totally on fire. Let's keep > it. > > # `CardTableBarrierSetAssembler::oop_store_at` in > `cardTableBarrierSetAssembler_x86.cpp` > > Same as before. But it's not even guarantee that `InlineTypePassFieldsAsArgs` > is true. We can have such a backtrace: > > CardTableBarrierSetAssembler::oop_store_at > (cardTableBarrierSetAssembler_x86.cpp:184) > CardTableBarrierSetAssembler::store_at > (cardTableBarrierSetAssembler_x86.cpp:90) > MacroAssembler::store_heap_oop (macroAssembler_x86.cpp:5515) > do_oop_store (templateTable_x86.cpp:148) > (called, for instance, from TemplateTable::putfield_or_static_helper > (templateTable_x86.cpp:2964)) > > where the last give `r8` for `tmp3`. It is not quite clear to me why we don't > have a problem in mainline, because it looks like corrupting address base > register is a bad idea given that we use it after. > > # `MacroAssembler::unpack_inline_helper` in `macroAssembler_aarch64.cpp` > > ## First part > https://github.com/openjdk/valhalla/blob/1077e4f9f06336af30d95fc28cbab4d31c9f2a44/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L7163-L7166 > > Yes, these registers are saved, as we saw above! I've added some asserts to > make sure that they are still in the `call_clobbered_gp_registers()` set. But > it seems a bit ... Thanks for the detailed analysis Marc! Changes look good to me, I just added a few comments. src/hotspot/cpu/aarch64/gc/shared/cardTableBarrierSetAssembler_aarch64.cpp line 117: > 115: if (!precise || (dst.index() == noreg && dst.offset() == 0)) { > 116: if (tmp3 != noreg) { > 117: // When tmp3 is given, we cannot corrupt the dst.base() register > (from MacroAssembler::pack_inline_helper or do_oop_store) > it doesn't look like an accident. If we remove the if-branch, tests are > totally on fire. Let's keep it. It's only the `__ mov(tmp3, dst.base());` that could potentially be removed, right? Not the entire branch. If the `mov` is still needed, should it be guarded by `InlineTypePassFieldsAsArgs`? src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 7172: > 7170: assert(clobbered_gp_regs.contains(tmp1), "tmp1 must be saved > explicitly if it's not a clobber"); > 7171: assert(clobbered_gp_regs.contains(tmp2), "tmp2 must be saved > explicitly if it's not a clobber"); > 7172: assert(clobbered_gp_regs.contains(r14), "r14 must be saved explicitly > if it's not a clobber"); Great idea with these asserts! src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp line 241: > 239: // Barriers might be emitted when converting between (scalarized) > calling conventions for inline > 240: // types. Save all argument registers before calling into the runtime. > 241: __ push_call_clobbered_registers(); I think this comment should just be reverted to the mainline state because it's not specific to the scalarized calling convention. ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1824#pullrequestreview-3597477751 PR Review Comment: https://git.openjdk.org/valhalla/pull/1824#discussion_r2634139562 PR Review Comment: https://git.openjdk.org/valhalla/pull/1824#discussion_r2634140381 PR Review Comment: https://git.openjdk.org/valhalla/pull/1824#discussion_r2634137724
