-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8350563: C2 compilation fails because PhaseCCP does not reach a fixpoint #23871
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
Conversation
|
👋 Welcome back cushon! A progress list of the required criteria for merging this PR into |
|
/contributor add Matthias Ernst [email protected] |
|
@cushon 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 73 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 |
|
@cushon |
Webrevs
|
|
@cushon @mernst-github Thanks for working on the fix! Can you please add a description to the PR (and if possible also on JIRA) to explain what the issue is, and how you are fixing it? |
|
Thanks for taking a look!
Done
Matthias reports: Re: regtest: I don't currently have a standalone regtest, I am using java/lang/Character/CheckProp.java from the issue report to reproduce the failure and verify the fix. I can try and extract the failure condition into a standalone test. |
eme64
left a comment
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 the updates!
I have some comments about the fix. Generally extending push_and is the right approach, I think :)
src/hotspot/share/opto/phaseX.cpp
Outdated
| if (new_type != nullptr && new_type->is_con()) { | ||
| // Pattern: parent (now constant) -> (ConstraintCast | ConvI2L)* -> And | ||
| to_push = parent; |
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.
Why do we need this?
Do we not already push all uses of parent further up?
1881 void PhaseCCP::push_child_nodes_to_worklist(Unique_Node_List& worklist, Node* n) const {
1882 for (DUIterator_Fast imax, i = n->fast_outs(imax); i < imax; i++) {
1883 Node* use = n->fast_out(i);
1884 push_if_not_bottom_type(worklist, use);
1885 push_more_uses(worklist, n, use);
1886 }
1887 }
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.
What am I missing?
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.
Ah, that would not traverse through ContraintCast and ConvI2L. I see.
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.
Ah, why not just add these two cases to the case below?
((use_op == Op_LShiftI || use_op == Op_LShiftL) && use->in(2) == parent
or
n->is_ConstraintCast() || n->Opcode() == Op_ConvI2L ?
That would allow you to traverse this pattern too:
// Pattern: parent (now constant) -> (ConstraintCast | ConvI2L)* -> And
I think this would be closer to existing patterns.
What do you think?
src/hotspot/share/opto/phaseX.cpp
Outdated
| push_if_not_bottom_type(worklist, n); | ||
| } | ||
| }; | ||
| to_push->visit_uses(push_and_uses_to_worklist, is_boundary); |
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.
And why not just call this line in two places, rather than having to work with to_push? Would that not be less code?
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.
Matthias reports:
That was purely a stylistic choice - the two cases can be integrated, we can push from "use" in both cases. Updated.
|
Updated to address review comments, and add a test |
eme64
left a comment
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 the updates! It looks much better :)
| @@ -0,0 +1,51 @@ | |||
| /* | |||
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.
The test should probably be moved to test/hotspot/jtreg/compiler/ccp/, that would be more specific.
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.
Done
| // Pattern: parent -> LShift (use) -> (ConstraintCast | ConvI2L)* -> And | ||
| ((use_op == Op_LShiftI || use_op == Op_LShiftL) && use->in(2) == parent)) { | ||
|
|
||
| auto push_and_uses_to_worklist = [&](Node* n) { |
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.
Amazing, this looks much better. I suggest you rename new_type -> parent_type, just to keep things consistent.
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.
Done
Co-authored-by: Emanuel Peter <[email protected]>
chhagedorn
left a comment
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.
Some small comments, otherwise, it looks good to me.
| * @run main/othervm -Xbatch -XX:-TieredCompilation compiler.c2.TestAndConZeroCCP | ||
| * @run driver compiler.c2.TestAndConZeroCCP | ||
| */ | ||
| package compiler.c2; |
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.
You should update this to ccp now that you moved the test and use main instead of driver. Otherwise, @run driver is never executed with additionally passed in flags, for example in higher tier.
| * @run main/othervm -Xbatch -XX:-TieredCompilation compiler.c2.TestAndConZeroCCP | |
| * @run driver compiler.c2.TestAndConZeroCCP | |
| */ | |
| package compiler.c2; | |
| * @run main/othervm -Xbatch -XX:-TieredCompilation compiler.ccp.TestAndConZeroCCP | |
| * @run driver compiler.ccp.TestAndConZeroCCP | |
| */ | |
| package compiler.ccp; |
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.
Done
|
|
||
| public class TestAndConZeroCCP { | ||
|
|
||
| public static void main(String[] args) { |
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.
You should use a 4 space indentation for Java tests.
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.
Done
Co-authored-by: Christian Hagedorn <[email protected]>
|
There was a test failure in a bigger test that I cannot share. But I was able to extract a simple reproducer: public class Test {
public static void main(String[] args) {
test();
}
static void test() {
Integer.parseInt("1");
}
}Run with (might need to run multiple times or increase Output: |
|
From Matthias Thank you Christian for that reproducer! Unless you've already tried, I think I'll first try and verify whether it's an already present issue at HEAD and this fix is simply incomplete (related to the CCP vs IGVN discussion on #22856 (comment)) or whether it's being caused by this fix PR. |
|
I've tried it with latest master but could not reproduce it. Adding nodes to the CCP worklist should not directly be an issue. So, I expect this patch just revealed an existing issue with JDK-8346664.
Yes, I guess we can continue with this patch to reduce the noise in the CI and follow up with another bug fix to address the problem I reported. Can you file a bug accordingly? |
322 Phi === 303 119 255 [[ 399 388 351 751 366 377 ]] #int:-256..127 !jvms: Integer::parseInt @ bci:151 (line 625) While this Phi dumps as "#int:-256..127", `phase->type(expr)` returns a type that is_con -256.
|
From Matthias I was able to reproduce the issue as is by Christian and I have a fix - with some caveats since I only have a partial understanding of what's happening. Here's what I know: In order to simplify EXPR & MASK, AndINode::Value() compares the trailing zero bits of EXPR against the width of MASK. What I observe for the Integer.parseInt reproducer is that Concretely, my understanding is that the node is "digit" here: I can only guess why we'd get an is_con() type for this node, I assume it's a speculative optimization, but I would expect that to happen all the time. Anyway, my first instinct is to replace the constant check in the optimization It does feel like it's addressing a symptom though, not a cause. Unfortunately, I have been unable to reproduce this outside of jl.Integer.parseInt. Even when copying jl.Integer.parseInt to my own method, I wasn't able to trigger the crash. Matthias |
|
Thanks Matthias for having a look at the issue and proposing a fix! While this fix seems to work, I think we should address it slightly differently with an explicit bailout, though. Let's step back a bit: CCP first sets all types to top and then tries to widen them (i.e. an optimistic approach) while IGVN does the opposite: We start by setting all types to bottom and then try to narrow them (i.e. a pessimistic approach). The assert we've faced in CCP complains that we tried to narrow some type again which is against the rules of CCP - we can only widen types. Now when CCP runs, we start with every type of every node at top. When visiting
That is perfectly fine. What happened here is that only one input of the phi with type As a consequence this can happen when having a phi and only looking at the currently tracked CCP types:
Let's look at the output of the failure: it looks like we first optimized When looking at the code, we check that jdk/src/hotspot/share/opto/mulnode.cpp Lines 2255 to 2260 in f25f701
But it looks like we miss that for jdk/src/hotspot/share/opto/mulnode.cpp Lines 2180 to 2185 in f25f701
So, if the type of jdk/src/hotspot/share/opto/mulnode.cpp Lines 185 to 187 in f25f701
We find top which is narrower than type zero and we fail with the assert. Long story short, you should check for I suggest to add the small reproducer as additional test case. |
A child phi node may transition from con to non-con, making the AND node transition back from "0" to its current type. If that current type is still TOP we're in violation of monotonicity. Therefore, don't apply optimization if AND is not integer yet.
|
From Matthias Thank you Christian for the excellent explanation. I understand how the phi going from con to non-con makes the AND node go from 0 back to top. I've pushed a commit with the fix and the reproducer; verified the latter only passes w/ the fix. |
chhagedorn
left a comment
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.
Sure, you're welcome! The fix looks good to me now. I will emit some internal testing again when you merged the tests together. Then I think it's good to go :-)
|
|
||
| public class TestAndConZeroMonotonic { | ||
|
|
||
| public static void main(String[] args) { |
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.
Since it's quite an easy test, I suggest to merge the two test files together by calling Integer::parseInt() directly instead. You can just add another @run statement.
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.
Done
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 the merge! I'll submit some testing and report back once it's finished.
Co-authored-by: Christian Hagedorn <[email protected]>
chhagedorn
left a comment
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.
Testing looked good, thanks for the updates!
|
From Matthias: Thank you all! Will integrate this now. I hope this addresses all remaining issues with the optimization. |
|
/integrate |
|
Going to push as commit 4954a33.
Your commit was automatically rebased without conflicts. |
Hello, please consider this fix for JDK-8350563 contributed by my colleague Matthias Ernst.
#22856 introduced a new
Value()optimization for the patternAndIL(Con, Mask).This optimization can look through CastNodes, and therefore requires additional logic in CCP to push these
transitive uses to the worklist.
The optimization is closely related to analogous optimizations for SHIFT nodes, and we also extend the existing logic for
CCP worklist handling: the current logic is "if the shift input to a SHIFT node changes, push indirect AND node uses to the CCP worklist".
We extend it by adding "if the (new) type of a node is an IntegerType that
is_con, ...to the predicate.Progress
Issue
Reviewers
Contributors
<[email protected]>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23871/head:pull/23871$ git checkout pull/23871Update a local copy of the PR:
$ git checkout pull/23871$ git pull https://git.openjdk.org/jdk.git pull/23871/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23871View PR using the GUI difftool:
$ git pr show -t 23871Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23871.diff
Using Webrev
Link to Webrev Comment