On Fri, 19 Dec 2025 08:15:29 GMT, Tobias Hartmann <[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 >> mak... > > 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`? I don't think we can just remove the `mov` part. I don't really understand how to interpret that. We have __ mov(tmp3, dst.base()); store_check(masm, tmp3, dst); do you suggest we write store_check(masm, tmp3, dst); ? But then, `tmp3` is not set to anything meaningful yet. Or store_check(masm, dst.base(), dst); But that is exactly the else-branch. Moreover, I've tried to guard and it makes some test fails. For instance, we can come from 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)) that doesn't need `InlineTypePassFieldsAsArgs` but will give `tmp3` and if we don't use it here (for instance, if I replace the condition by `InlineTypePassFieldsAsArgs && tmp3 != noreg`), we get various test failures. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1824#discussion_r2634178710
