RFR: 8349452: Fix performance regression for Arrays.fill() with AVX512 [v10]
Srinivas Vamsi Parasa
sparasa at openjdk.org
Wed Dec 24 22:06:48 UTC 2025
On Thu, 18 Dec 2025 23:53:08 GMT, Derek White <drwhite at openjdk.org> wrote:
> No correctness issues, but some cleanups suggested. Thanks!
Thank you, Derek (@dwhite-intel) for your review comments and suggestions! Please see the updated code incorporating your suggestions.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 5800:
>
>> 5798: Register rtmp, Register rtmp2, XMMRegister xtmp) {
>> 5799: ShortBranchVerifier sbv(this);
>> 5800: assert_different_registers(to, value, count, rtmp);
>
> Should add rtmp2 to assert_different_registers also
Thanks for catching that! Please see this fixed in the updated code.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 9339:
>
>> 9337: Label L_exit;
>> 9338: Label L_fill_start;
>> 9339: Label L_fill_32_tail;
>
> This is a bit more style/readability suggestion:
> I think these labels can move to the block starting on line 9387
>
> L_fill_start, L_fill_32_tail, L_fill_96_bytes,
> L_fill_128_bytes,
> L_fill_128_bytes_loop,
> L_fill_128_bytes_loop_header,
> L_fill_128_bytes_loop_pre_header
Please see these labels moved to the appropriate places as suggested to improve readability.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 9340:
>
>> 9338: Label L_fill_start;
>> 9339: Label L_fill_32_tail;
>> 9340: Label L_fill_64_tail;
>
> Similar suggestion: L_fill_64_tail can move to block starting on line 9462
Please see the label L_fill_64_tail moved as suggested.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 9341:
>
>> 9339: Label L_fill_32_tail;
>> 9340: Label L_fill_64_tail;
>> 9341: Label L_fill_64_bytes;
>
> L_fill_64_bytes is now unused
True, L_fill_64_bytes is now unused is unused now. Please see it removed in the updated code.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 9345:
>
>> 9343: Label L_fill_128_bytes;
>> 9344: Label L_fill_128_bytes_loop;
>> 9345: Label L_fill_128_loop_header;
>
> L_fill_128_bytes_loop_header is unused (preexisting?)
L_fill_128_bytes_loop_header is a preexisting label and is being used.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/28442#issuecomment-3690547891
PR Review Comment: https://git.openjdk.org/jdk/pull/28442#discussion_r2646294220
PR Review Comment: https://git.openjdk.org/jdk/pull/28442#discussion_r2646296508
PR Review Comment: https://git.openjdk.org/jdk/pull/28442#discussion_r2646296960
PR Review Comment: https://git.openjdk.org/jdk/pull/28442#discussion_r2646294502
PR Review Comment: https://git.openjdk.org/jdk/pull/28442#discussion_r2646295332
More information about the hotspot-dev
mailing list