Skip to content
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

x86: Increase inline reference arraycopy threshold #20525

Merged
merged 1 commit into from
Nov 13, 2024
Merged

x86: Increase inline reference arraycopy threshold #20525

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions runtime/compiler/x/codegen/J9TreeEvaluator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1899,10 +1899,10 @@ TR::Register *J9::X86::TreeEvaluator::arraycopyEvaluator(TR::Node *node, TR::Cod
comp->target().cpu.supportsAVX() &&
comp->target().is64Bit();

int32_t repMovsThresholdBytes = 32;
int32_t repMovsThresholdBytes = comp->target().cpu.supportsFeature(OMR_FEATURE_X86_AVX512F) ? 128 : 64;
int32_t newThreshold = comp->getOptions()->getArraycopyRepMovsReferenceArrayThreshold();

if ((repMovsThresholdBytes < newThreshold) && ((newThreshold == 64) || (newThreshold == 128)))
if ((newThreshold == 32) || (newThreshold == 64) || (newThreshold == 128))
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember being confused by this code when it was first delivered, and with the passage of time I am once again confused.

The documentation for the arraycopyRepMovsReferenceArrayThreshold option says that only 32, 64, or 128 bytes are supported (32 is the default). This means that this if branch should always be taken because the condition on L1905 will always be true. This also means that repMovsThresholdBytes will always be overwritten from what it was set to on L1902.

Is the purpose of the code on L1902 then just in case someone provided an unsupported value for arraycopyRepMovsReferenceArrayThreshold ? If so, I think the code could be written a bit differently to convey that. Specifically, move L1902 as an else to this if and add a comment saying that this path is for when an invalid reference array threshold is provided.

Or am I completely wrong in understanding this logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, maybe I have to view this from the perspective of your new OMR PR 7534 alongside this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my bad, I should have mentioned that this PR should be reviewed together with OMR PR #7534.

Without OMR PR #7534, this PR won't break the build, but it does depend on OMR PR #7534 to get the reference arraycopy threshold properly increased to 64/128.

In OMR PR #7534, the default values for all arraycopyRepMovs*ArrayThresholds are reset to 0 in OMROptions.hpp.

       _arraycopyRepMovsByteArrayThreshold = 0;
       _arraycopyRepMovsCharArrayThreshold = 0;
       _arraycopyRepMovsIntArrayThreshold = 0;
       _arraycopyRepMovsLongArrayThreshold = 0;
       _arraycopyRepMovsReferenceArrayThreshold = 0;

My intended logic is
(1) CG should set the threshold to the proper value: 64 or 128
(2) If someone sets a value in -Xjit options, the threshold will get updated only if the value is valid

{
// If the CPU doesn't support AVX512, reduce the threshold to 64 bytes
repMovsThresholdBytes = ((newThreshold == 128) && !comp->target().cpu.supportsFeature(OMR_FEATURE_X86_AVX512F)) ? 64 : newThreshold;
Expand Down