On Fri, 19 Dec 2025 08:30:37 GMT, Marc Chevalier <[email protected]> wrote:
>> 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.
I don't understand why it's not a problem on mainline, tho. And I agree it's
weird. I've looked, but it's hard to investigate why something works rather
than not (on a somewhat diverging codebase). I spent some time on that, but
eventually decided it's not that critical, as long as we have a way to make it
work for Valhalla too.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1824#discussion_r2634193195
