-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8333393: PhaseCFG::insert_anti_dependences can fail to raise LCAs and to add necessary anti-dependence edges #23691
base: master
Are you sure you want to change the base?
8333393: PhaseCFG::insert_anti_dependences can fail to raise LCAs and to add necessary anti-dependence edges #23691
Conversation
👋 Welcome back dlunden! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Pinging the reviewers of the prior pull request: @chhagedorn @eme64 @robcasloz @merykitty /contributor add rcastanedalo |
@dlunde |
@dlunde |
@dlunde |
Webrevs
|
Issue Summary
When searching for load anti-dependences in GCM, the memory state for the load is sometimes represented not only by the memory node input of the load, but also other memory nodes. Because PhaseCFG::insert_anti_dependences searches for anti-dependences only from the load's memory input, it is, therefore, possible to sometimes overlook anti-dependences. The result is that loads are potentially scheduled too late, after stores that redefine the memory states of the loads.
Changeset
It is not yet clear why multiple nodes sometimes represent the memory state of a load, nor if this is expected. We can, however, resolve all the miscompiled test cases seen in this issue by improving the idealization of Phi nodes. Specifically, there is an idealization where we split Phis through input MergeMems, that we, prior to this changeset, applied too conservatively.
To illustrate the idealization and how it resolves this issue, consider the example below.
64 membar_release
is a critical anti-dependence for183 loadI
. The anti-dependence search starts at the load's direct memory input,107 Phi
, and stops immediately at Phis. Therefore, the search ends at106 Phi
and we never find64 membar_release
.We can apply the split-through-MergeMem Phi idealization to
119 Phi
. This idealization pushes119 Phi
through120 MergeMem
and121 MergeMem
, splitting it into the individual inputs of the MergeMems in the process. As a result, we replace119 Phi
with two new Phis. One of these generated Phis has identical inputs to107 Phi
(106 Phi
and104 Phi
), and further idealizations will merge this new Phi and107 Phi
. As a result,107 Phi
then has a Phi-free path to64 membar_release
and we correctly discover the anti-dependence.The changeset consists of the following changes.
ResourceMark
inPhiNode::split_out_instance
.TestGCMLoadPlacement.java
.For reference, here is a previous PR with an alternative fix that we decided to discard in favor of the fix in this PR.
Testing
tier1
totier4
(and additional Oracle-internal testing) on Windows x64, Linux x64, Linux aarch64, macOS x64, and macOS aarch64.Progress
Issue
Contributors
<rcastanedalo@openjdk.org>
<chagedorn@openjdk.org>
<thartmann@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23691/head:pull/23691
$ git checkout pull/23691
Update a local copy of the PR:
$ git checkout pull/23691
$ git pull https://git.openjdk.org/jdk.git pull/23691/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23691
View PR using the GUI difftool:
$ git pr show -t 23691
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23691.diff
Using Webrev
Link to Webrev Comment