8349452: Fix performance regression for Arrays.fill() with AVX512 #28442
Conversation
|
👋 Welcome back sparasa! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@vamsi-parasa The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| fill64(Address(dst, disp), xmm, use64byteVector); | ||
| } | ||
|
|
||
| void MacroAssembler::fill32_unmasked(uint shift, Register dst, int disp, XMMRegister xmm, |
There was a problem hiding this comment.
This could be called as fill32_tail. Also good to replace overall fill32_masked with fill32_tail.
There was a problem hiding this comment.
Please see this suggestion incorporated in the updated code.
| bind(L_done); | ||
| } | ||
|
|
||
| void MacroAssembler::fill64_unmasked(uint shift, Register dst, int disp, |
There was a problem hiding this comment.
This could be called as fill64_tail. Also good to replace overall fill64_masked with fill64_tail.
There was a problem hiding this comment.
Please see this suggestion incorporated in the updated code.
There was a problem hiding this comment.
Thanks @vamsi-parasa. It will be also good to remove fill64_masked and fill32_masked overall.
| fill32_unmasked(shift, to, 0, xtmp, count, rtmp); | ||
| jmp(L_exit); |
There was a problem hiding this comment.
Instead of repeating fill32_unmasked multiple time, you could jmp to say L_fill_32_tail and have the fill32_unmasked code there one time.
There was a problem hiding this comment.
Please see this suggestion incorporated in the updated code.
| fill64(to, 0, xtmp); | ||
| subq(count, 64 >> shift); | ||
| fill32_masked(shift, to, 64, xtmp, k2, count, rtmp); | ||
| fill32_unmasked(shift, to, 64, xtmp, count, rtmp); |
There was a problem hiding this comment.
Instead of repeating fill64_unmasked multiple time, you could jmp to say L_fill_64_tail and have the fill64_unmasked code there one time.
There was a problem hiding this comment.
Please see this suggestion incorporated in the updated code.
|
The pre-submit test seem to be unrelated to the PR changes. A fresh merge with tip might resolve those. |
2 participants
The goal of this PR is to fix the performance regression in Arrays.fill() x86 stubs caused by masked AVX stores. The fix is to replace the masked AVX stores with store instructions without masks (i.e. unmasked stores).
fill32_masked()andfill64_masked()stubs are replaced withfill32_unmasked()andfill64_unmasked()respectively.To speedup unmasked stores, array fills for sizes < 64 bytes are broken down into sequences of 32B, 16B, 8B, 4B, 2B and 1B stores, depending on the size.
Performance comparison for byte array fills in a loop for 1 million times
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28442/head:pull/28442$ git checkout pull/28442Update a local copy of the PR:
$ git checkout pull/28442$ git pull https://git.openjdk.org/jdk.git pull/28442/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28442View PR using the GUI difftool:
$ git pr show -t 28442Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28442.diff
Using Webrev
Link to Webrev Comment