Skip to content

Intrinsic for AArch64 Arrays.fill(array, constant) #10207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 29, 2025
Merged

Intrinsic for AArch64 Arrays.fill(array, constant) #10207

merged 1 commit into from
Jan 29, 2025

Conversation

JohnTortugo
Copy link
Contributor

@JohnTortugo JohnTortugo commented Dec 2, 2024 •
edited
Loading

Please, consider this patch to intrinsify Arrays.fill(array, constant) for byte, short and int on AArch64 backend. The assembly code is a port of the HotSpot assembly for the same intrinsic. Thank you @gergo- for reviewing the draft version of this PR!

The below is a summary of some quick JMH tests that I ran on my dev machine.

Instr     Benchmark                       (size)   Mode  Cnt          Score          Error  Units
No        ArrayFillBenchmark.fill_bytes       16  thrpt    5  130,858,263.770 ±  7175243.892  ops/s
Yes       ArrayFillBenchmark.fill_bytes       16  thrpt    5  268,367,821.486 ±  2717945.345  ops/s
No        ArrayFillBenchmark.fill_bytes     1024  thrpt    5    7,284,374.553 ±   131421.427  ops/s
Yes       ArrayFillBenchmark.fill_bytes     1024  thrpt    5   94,448,799.400 ±  1096754.457  ops/s
No        ArrayFillBenchmark.fill_bytes     4096  thrpt    5    1,814,548.662 ±    33455.246  ops/s
Yes       ArrayFillBenchmark.fill_bytes     4096  thrpt    5   28,970,761.402 ±  2636159.892  ops/s

No        ArrayFillBenchmark.fill_shorts      16  thrpt    5  137,210,363.401 ±  6208453.450  ops/s
Yes       ArrayFillBenchmark.fill_shorts      16  thrpt    5  285,341,885.029 ± 11301099.208  ops/s
No        ArrayFillBenchmark.fill_shorts    1024  thrpt    5    7,178,718.150 ±   741170.216  ops/s
Yes       ArrayFillBenchmark.fill_shorts    1024  thrpt    5   53,996,849.866 ±  2353229.277  ops/s
No        ArrayFillBenchmark.fill_shorts    4096  thrpt    5    1,814,690.509 ±    13988.852  ops/s
Yes       ArrayFillBenchmark.fill_shorts    4096  thrpt    5   14,905,037.102 ±  1719260.337  ops/s

No        ArrayFillBenchmark.fill_ints        16  thrpt    5  132,083,594.942 ±  7995181.451  ops/s
Yes       ArrayFillBenchmark.fill_ints        16  thrpt    5  374,192,809.496 ±  5778630.471  ops/s
No        ArrayFillBenchmark.fill_ints      1024  thrpt    5    7,044,324.507 ±   130830.618  ops/s
Yes       ArrayFillBenchmark.fill_ints      1024  thrpt    5   28,677,308.237 ±  1515323.358  ops/s
No        ArrayFillBenchmark.fill_ints      4096  thrpt    5    1,648,798.029 ±     9113.592  ops/s
Yes       ArrayFillBenchmark.fill_ints      4096  thrpt    5    7,563,740.695 ±   627888.347  ops/s

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 2, 2024
@gergo- gergo- self-assigned this Dec 5, 2024
gergo-
gergo- reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@gergo- gergo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping up this work! I left some high-level comments for now. I'll do some testing and a more thorough review of the assembly code soon.

@JohnTortugo
Copy link
Contributor Author

@gergo- - I believe I've addressed all your comments so far. Please, let me know if you have any other concerns.

gergo-
gergo- reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@gergo- gergo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some requests for improved documentation, and the value to be filled should be a long throughout. I also started some checks on this PR, note that the style checks had some complaints. Please make sure mx checkstyle passes in the compiler suite.

This will soon be ready for internal testing.

@JohnTortugo
Copy link
Contributor Author

JohnTortugo commented Jan 9, 2025 •
edited
Loading

@gergo- - Please take a look again when you have some time. I've addressed your latest feedback.

@gergo-
Copy link
Member

gergo- commented Jan 14, 2025

Thanks @JohnTortugo, I've started some tests and will try to take a detailed look at the code in the next few days.

@gergo-
Copy link
Member

gergo- commented Jan 14, 2025

@JohnTortugo in the meantime, please take a look at the failing tests at https://github.com/oracle/graal/actions/runs/12700274525/job/35582509603?pr=10207
Your new tests should probably be skipped when running on AMD64 since we won't use the new intrinsic nodes on this platform.

gergo-
gergo- reviewed Jan 16, 2025
View reviewed changes
Copy link
Member

@gergo- gergo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good now, thanks! I'll start internal testing and ask another pair of eyes for another look.
Please squash all your commits into one and force-push onto this branch. Unlike the JDK's GitHub repository, ours does not automatically squash on merge.

@JohnTortugo
Copy link
Contributor Author

Looks pretty good now, thanks! I'll start internal testing and ask another pair of eyes for another look.
Please squash all your commits into one and force-push onto this branch. Unlike the JDK's GitHub repository, ours does not automatically squash on merge.

Done. Thank you for reviewing!

@JohnTortugo
Copy link
Contributor Author

I addressed latest comments, squashed all commits and force pushed again.

@JohnTortugo JohnTortugo force-pushed the array_fill branch 2 times, most recently from 0099aaf to 9655ec3 Compare January 18, 2025 18:05
@gergo-
Copy link
Member

gergo- commented Jan 20, 2025

Thanks for the latest changes, @JohnTortugo . Internal tests mostly pass but there is one error that I need to look into, I'll keep you posted.

@gergo-
Copy link
Member

gergo- commented Jan 21, 2025

Tests look good, I needed a small internal adjustment in our EE code. I'll assign extra reviewers as required by our policies, then hopefully we can merge this soon.

@JohnTortugo
Copy link
Contributor Author

Thank you @gergo- !

@JohnTortugo
Copy link
Contributor Author

All comments addressed. Looking forward to merge this!

gergo-
gergo- approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@gergo- gergo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for all this work, this PR is in the merge queue.

@graalvmbot graalvmbot merged commit 9ad216f into oracle:master Jan 29, 2025
13 checks passed
@JohnTortugo JohnTortugo mentioned this pull request Jan 30, 2025
Open
@JohnTortugo JohnTortugo deleted the array_fill branch February 10, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teshull teshull teshull left review comments

@mur47x111 mur47x111 mur47x111 left review comments

@gergo- gergo- gergo- approved these changes

Assignees

@gergo- gergo-

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JohnTortugo @gergo- @teshull @mur47x111 @graalvmbot