-
Notifications
You must be signed in to change notification settings - Fork 392
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.
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
x86-64: Implement an enhancement for byte array and char array System.arraycopy #7332
x86-64: Implement an enhancement for byte array and char array System.arraycopy #7332
Conversation
@0xdaryl May I ask you to review this change? Thank you! |
x86-64 macOS failure is a known issue #7181
|
Can you attach a JIT log for a method that shows the code generated for these functions? Just a tracecg of a single method should be fine. Thanks. |
Search |
Generally, I think the instruction sequences are reasonable. Please consider Brad’s suggestions where appropriate for using wider instructions on hardware that supports it. My main feedback concerns the naming of these optimizations. Calling them “enhancements”, while technically correct, is too generic for someone who wasn’t involved in its development up front. There aren’t even any code comments to describe what the enhancements are, leaving others to read and understand the code to learn about them. I suggest incorporating something like “inlineSmallArraycopiesWithoutSTOS” into the naming would make it better. For example, “inlineSmall16BitPrimitiveArraycopiesWithoutSTOS” Longer term, I’m not convinced that inlining this much code at each arraycopy call (over 50 instructions) is worth the simplicity and savings from calling out to a helper to do the same thing. I’m willing to let the current inline design go through now, but I would like a helper approach evaluated for the future. This might be fine for a method with one or two arraycopies, but if there are several arraycopies in a method (and we do see methods like that) the code footprint will add up quickly. |
I'm currently running sanity test on the changes that address the above comments. I'll push another update once the sanity test looks good |
6cc0763
to
9065569
Compare
@0xdaryl Comments are addressed. Ready for another review. Thank you!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the comments describing the operation. They look good!
I missed publishing a couple of other comments in my earlier review for some reason. They are minor changes only.
I realize the test farm isn't in great shape right now. Can you confirm you tested this code internally on both 64-bit and 32-bit?
@BradleyWood : please approve if your comments are addressed |
I tested only on 64-bit machines since I thought the enhancement was guarded with checking if target is 64-bit. I'll test on 32-bit as well after addressing the latest comments.
|
The setup to run `rep movsd` is not efficient on copying smaller sizes. The enhancement inlines copy size equal or less than 64 bytes without using `rep movsd`. Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
The setup to run `rep movsb` is not efficient on copying smaller sizes. The enhancement inlines copy size equal or less than 64 bytes without using `rep movsb`. Co-Authored-By: Henry Zongaro <zongaro@ca.ibm.com> Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
`disableArrayCopyByteArrayInlineSmallSizeWithoutREPMOVS` - Disable array copy enhancement for 8 bit primitive array `disableArrayCopyCharArrayInlineSmallSizeWithoutREPMOVS` - Disable array copy enhancement for 16 bit primitive array Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
9065569
to
a81d5c0
Compare
@0xdaryl Addressed all review comments. Ready for another review. Thank you!
|
@vijaysun-omr : FYI |
AArch64 codegen inlines arraycopy only when the copy length is a constant at the compile time. Otherwise it generates instructions to call arraycopy helpers in https://github.com/eclipse/omr/blob/master/compiler/aarch64/runtime/ARM64arrayCopy.spp |
Implement an enhancement for byte array and char array System.arraycopy on x86-64.
Also add JIT options that disable array copy enhancement:
disableArrayCopyEnhancementByteArray
: Disable array copy enhancement for 8 bit primitive arraydisableArrayCopyEnhancementCharArray
: Disable array copy enhancement for 16 bit primitive array