8280799: С2: assert(false) failed: cyclic dependency prevents range check elimination #7319
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
I executed some testing and hit the following failure with the And with |
|
@TobiHartmann thanks for running testing. I reproduced one of them (thanks for helping with that too) and fixed it. Would you mind running testing again? |
|
Sure, I'll re-run testing and report back. |
TobiHartmann
approved these changes
Feb 11, 2022
|
@rwestrel This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 44 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
vnkozlov
reviewed
Feb 11, 2022
| new_ctrl = c; | ||
| } | ||
| least = new_ctrl; | ||
| } | ||
| // Try not to place code on a loop entry projection | ||
| // which can inhibit range check elimination. | ||
| if (least != early) { |
There was a problem hiding this comment.
It is the same check as new above - you can combine these scopes.
vnkozlov
approved these changes
Feb 11, 2022
|
Thanks @TobiHartmann and @vnkozlov for the reviews. |
|
/integrate |
|
Going to push as commit 1ef45c5.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
Regression: https://bugs.openjdk.java.net/browse/JDK-8281811 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The loop exit condition of the test method:
if (i == stop) {
break;
}
requires insertion of a loop limit predicate when the loop is turned
into a counted loop. stop is a LoadI. Next:
array[stop - i + j] = 0;
is transformed to:
array[stop - i] = 0;
and the range check for that array access becomes candidate for
predication in a subsequent loop opts pass. stop has control just
above the loop limit check when that happens (because it is assigned
control as late as possible). But the loop predicate for the bound
check needs to be above the loop limit check and that causes the
assert failure.
There's already logic in PhaseIdealLoop::build_loop_late_post_work()
to assign control to nodes above predicates so this sort of issues
doesn't occur. But it only applies to node initially on the entry
projection right above the loop head. The fix I propose is to remove
that restriction.
That logic was added by JDK-8203197 and looking back at this change I
noticed I replaced some existing logic with the current logic but,
while the 2 overlap, the current logic is not guaranteed to always
cover some cases handled by the old logic. So I resurrected that old
logic here.
Finally, when running tests, I hit failures because Opaque nodes for
skeleton predicates can now end up above a predicate that is split
thru phi. That causes the Opaque nodes to be split up and breaks
pattern matching. I'm actually not sure this issue is specific to the
change here so I think it's best to treat it as a general issue and
fix it by cloning the chain of nodes that lead to the Opaque node
down.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7319/head:pull/7319$ git checkout pull/7319Update a local copy of the PR:
$ git checkout pull/7319$ git pull https://git.openjdk.java.net/jdk pull/7319/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7319View PR using the GUI difftool:
$ git pr show -t 7319Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7319.diff