On Fri, 12 Jul 2024 09:32:44 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update arguments.cpp
>
> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 318:
>
>> 316:
>> 317: // Loop after unrolling, advance iterator.
>> 318: increment(t3_t, in_bytes(OMCache::oop_to_oop_difference()));
>
> Maybe I am misreading this but... in the unroll loop you avoid emitting the
> increment on the last iteration, but then you emit it explicitely here?
> Wouldn't it be cleaner to do it in the unroll loop always and elide the
> explicit increment after loop?
You are correct. It is a leftover from when it was possible to tweak the number
of unrolled lookups as well as whether it should loop the tail. Fixed.
> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 343:
>
>> 341: const Register t3_owner = t3;
>> 342: const ByteSize monitor_tag = in_ByteSize(UseObjectMonitorTable ? 0
>> : checked_cast<int>(markWord::monitor_value));
>> 343: const Address owner_address{t1_monitor,
>> ObjectMonitor::owner_offset() - monitor_tag};
>
> That may be just me, but I found that syntax weird. I first needed to look-up
> what the {}-initializer actually means. Hiccups like this reduce readability,
> IMO. I'd prefer the normal ()-init for the Address like we seem to do
> everywhere else.
I see. I tend to prefer uniform initialization as it makes narrowing
conversions illegal.
I remember `uniform initialization` coming up in some previous PR as well. It
is really only neccesary for some types of templated code, but it does also
makes easier to not make mistakes in the general case (as long as you avoid
`std::initializer_list`, which I think we explicitly forbid in our coding
guidelines).
I do not recall what the conclusion of that discussion was. But maybe it was
that this feature is to exotic and foreign for hotspot.
I prefer it tough. Even if I fail to consistently use it.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 674:
>
>> 672:
>> 673: // Loop after unrolling, advance iterator.
>> 674: increment(t, in_bytes(OMCache::oop_to_oop_difference()));
>
> Same issue as in aarch64 code.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1675676768
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1675676879
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1675677068
