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

Update to use the new LoadStoreHandler API on Power #11305

Merged
merged 7 commits into from
Feb 17, 2021
Merged

Update to use the new LoadStoreHandler API on Power #11305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e7c26c2
Disable prefetching for aloadi nodes on 64-bit
aviansie-ben Dec 3, 2020
a98159d
Add OMRLoadStoreHandler.cpp to builds
aviansie-ben Nov 12, 2020
7322e41
Update write barriers to use the new LoadStoreHandler API
aviansie-ben Nov 27, 2020
46fc477
Update read barriers to use the new LoadStoreHandler API
aviansie-ben Nov 27, 2020
bf696f2
Update simple read monitors to use the new LoadStoreHandler API
aviansie-ben Nov 27, 2020
b93e826
Update copyright dates to 2021
aviansie-ben Jan 19, 2021
f8d77f9
Remove assert at Julian's request
aviansie-ben Feb 11, 2021
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
3 changes: 2 additions & 1 deletion runtime/compiler/build/files/target/p.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2000, 2020 IBM Corp. and others
# Copyright (c) 2000, 2021 IBM Corp. and others
#
# This program and the accompanying materials are made available under
# the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -28,6 +28,7 @@ JIT_PRODUCT_BACKEND_SOURCES+=\
omr/compiler/p/codegen/OMRInstOpCode.cpp \
omr/compiler/p/codegen/OMRInstruction.cpp \
omr/compiler/p/codegen/OMRLinkage.cpp \
omr/compiler/p/codegen/OMRLoadStoreHandler.cpp \
omr/compiler/p/codegen/OMRMachine.cpp \
omr/compiler/p/codegen/OMRMemoryReference.cpp \
omr/compiler/p/codegen/OMRPeephole.cpp \
Expand Down
6 changes: 3 additions & 3 deletions runtime/compiler/p/codegen/J9CodeGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2020 IBM Corp. and others
* Copyright (c) 2000, 2021 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -403,7 +403,7 @@ J9::Power::CodeGenerator::insertPrefetchIfNecessary(TR::Node *node, TR::Register
static bool disableStringObjPrefetch = (feGetEnv("TR_DisableStringObjPrefetch") != NULL);
bool optDisabled = false;

if (node->getOpCodeValue() == TR::aloadi ||
if ((node->getOpCodeValue() == TR::aloadi && !comp->target().is64Bit()) ||
(comp->target().is64Bit() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like conditions being misplaced originally. This current fix certainly is not in line with the intention of prefetch optimization. Prefetch surely is beneficial performance-wise to 64bit non-compressed.

The conditions should read something like: if (is-objectReference-load && hotness-testing && CPU-testing)

Applies to the next change too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree that prefetching certainly could be applied in these circumstances, extending the prefetching optimization to handle this case correctly is out-of-scope for this PR. As the commit message clarifies, the previous implementation was not performing prefetching for such nodes either, but the way this was being achieved was slightly different. This change does not make prefetching not occur where it previously would, but simply makes insertPrefetchIfNecessary safe to call on aloadi nodes under 64-bit, as it would otherwise segfault.

Previously, we were simply failing to call insertPrefetchIfNecessary at all under the problematic conditions, thus sidestepping the problem. In commoning up code between the load evaluators into LoadStoreHandler, all of these evaluators need to behave uniformly and thus now call insertPrefetchIfNecessary unconditionally, with it being up to that method alone to make the decision as to whether to perform prefetching. This also avoids leaking implementation details of OpenJ9's prefetching into OMR.

If you'd like, I can open an issue for extending this support in the future, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

certainly need a follow-up item

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #11803

comp->useCompressedPointers() &&
node->getOpCodeValue() == TR::l2a &&
Expand Down Expand Up @@ -631,7 +631,7 @@ J9::Power::CodeGenerator::insertPrefetchIfNecessary(TR::Node *node, TR::Register
}
}

if (node->getOpCodeValue() == TR::aloadi ||
if ((node->getOpCodeValue() == TR::aloadi && !comp->target().is64Bit()) ||
(comp->target().is64Bit() &&
comp->useCompressedPointers() &&
(node->getOpCodeValue() == TR::iloadi || node->getOpCodeValue() == TR::irdbari) &&
Expand Down
Loading