-
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
8238812: assert(false) failed: bad AD file #2758
Conversation
👋 Welcome back rraghavan! A progress list of the required criteria for merging this PR into |
@r-v-raghav The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
src/hotspot/share/opto/ifnode.cpp
Outdated
hi = TypeInt::INT->_hi; | ||
break; | ||
default: | ||
assert(false, "should not reach here"); |
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.
Use ShouldNotReachHere();
instead.
src/hotspot/share/opto/ifnode.cpp
Outdated
// in-between the CmpNode(s) and the common value we compare. | ||
// Check for the following cases. Then return common value compared | ||
// if present and also save the constant values to be adjusted or | ||
// subtracted due to the possible AddINode in-between. |
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.
I would replace adjusted or subtracted
and say something like ... and also save the constant value that is added to infer the type of the common value we compare.
src/hotspot/share/opto/ifnode.cpp
Outdated
|
||
Node* res_val = NULL; | ||
this_adj_val = 0; | ||
dom_adj_val = 0; |
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 caller already initialized these to 0
, right?
src/hotspot/share/opto/ifnode.cpp
Outdated
Node* IfNode::get_base_comparing_value (Node* dom_if, PhaseIterGVN* igvn, jint& this_adj_val, jint& dom_adj_val) { | ||
Node* this_cmp = in(1)->in(1); | ||
Node* dom_cmp = dom_if->in(1)->in(1); | ||
assert(this_cmp->is_Cmp() != false && dom_cmp->is_Cmp() != false, "compare expected"); |
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.
No need to compare against false. This should be assert(this_cmp->is_Cmp() && dom_cmp->is_Cmp(), ...
but instead you could also simply replace declarations above by Node* this_cmp = in(1)->in(1)->as_Cmp()
which would include the assert.
src/hotspot/share/opto/ifnode.cpp
Outdated
Node* dom_val = dom_cmp->in(1); | ||
Node* this_val = this_cmp->in(1); | ||
if (this_val == dom_val) { | ||
res_val = this_val; |
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.
Add comments describing that this is Variant 1, same for other if branches below.
src/hotspot/share/opto/ifnode.cpp
Outdated
res_val = this_val; | ||
} else if (this_val->is_Add() && this_val->in(1) && this_val->in(1) == dom_val) { | ||
const TypeInt* val_t = igvn->type(this_val->in(2))->isa_int(); | ||
if (val_t && val_t->is_con()) { |
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.
val_t
implicitly casts a pointer to a boolean, should be replaced by val_t != NULL
. Same below.
src/hotspot/share/opto/ifnode.cpp
Outdated
jint dom_adj_val = 0; | ||
Node* n = NULL; | ||
|
||
n = get_base_comparing_value(dom_if, igvn, this_adj_val, dom_adj_val); |
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.
No need to initialize n
above. Should be Node* n = get_base_comparing_value(...
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.
Actually, you are not even using n
and can therefore just return a boolean from get_base_comparing_value
and simplify that method accordingly.
src/hotspot/share/opto/ifnode.cpp
Outdated
|
||
// Check if dominating if determines the result of this if | ||
bool IfNode::fold_dominated_if(ProjNode* proj, PhaseIterGVN* igvn) { | ||
Node* this_cmp = in(1)->in(1); |
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.
We only ever use this_cmp->in(1)
so you can replace this by Node* this_val = in(1)->in(1)->in(1)
. Same for dom_cmp
below.
src/hotspot/share/opto/ifnode.cpp
Outdated
failtype = dom_cmp->in(1)->as_Add()->add_ring(failtype, TypeInt::make(-dom_adj_val))->is_int(); | ||
} | ||
for (int i = 0; i < 2; ++i) { | ||
const TypeInt* type2 = filtered_int_type(igvn, this_cmp->in(1), proj_out(i)); |
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 not just name this type
?
src/hotspot/share/opto/ifnode.cpp
Outdated
assert(this_cmp->in(1)->is_Add() != false, "sanity"); | ||
type2 = this_cmp->in(1)->as_Add()->add_ring(type2, TypeInt::make(-this_adj_val))->is_int(); | ||
} | ||
const TypeInt* res_type = failtype->join(type2)->is_int(); |
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.
No need to declare res_type
, just use type
.
Webrevs
|
src/hotspot/share/opto/ifnode.cpp
Outdated
Node* this_cmp = in(1)->in(1)->as_Cmp(); | ||
Node* dom_cmp = dom_if->in(1)->in(1)->as_Cmp(); |
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.
Could be directly turned into an assertion with is_Cmp()
instead of using as_Cmp()
.
src/hotspot/share/opto/ifnode.cpp
Outdated
if (this_val == dom_val) { | ||
// Variant 1 | ||
return true; | ||
} else if (this_val->is_Add() && this_val->in(1) != NULL && this_val->in(1) == dom_val) { |
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.
As this_val
and dom_val
are both non-null: this_val->in(1) != NULL
is always true when this_val->in(1) == dom_val
holds and thus this_val->in(1) != NULL
can be removed. This can also be applied in Variant 3.
src/hotspot/share/opto/ifnode.cpp
Outdated
dom_adj_val = val_t->get_con(); | ||
return true; | ||
} | ||
} else if (this_val->is_Add() && dom_val->is_Add() && this_val->in(1) != NULL && dom_val->in(1) != NULL && this_val->in(1) == dom_val->in(1)) { |
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.
Here I think you can just remove one of the non-null checks (in case both are NULL).
src/hotspot/share/opto/ifnode.cpp
Outdated
@@ -837,8 +857,94 @@ bool IfNode::has_only_uncommon_traps(ProjNode* proj, ProjNode*& success, ProjNod | |||
return false; | |||
} | |||
|
|||
// There might be an AddINode (marked with *) with a constant increment |
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.
Do we only need to do this for AddNodes
? What about SubNodes
for example?
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.
Never found a SubNode in this case. Always AddI node with integer constant as second input (this constant
can be positive or negative values)
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.
That's because we always convert SubINodes
with constant second operand to AddINodes
:
jdk/src/hotspot/share/opto/subnode.cpp
Line 181 in 3d3eb5c
// Convert "x-c0" into "x+ -c0". |
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.
Okay I see, thanks for the explanation.
src/hotspot/share/opto/ifnode.cpp
Outdated
if (is_unsigned && lo >= 0) { | ||
// val u< cmp2 only passes for val >= 0 if cmp2 >= 0 | ||
lo = 0; | ||
} else { | ||
lo = TypeInt::INT->_lo; | ||
} |
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.
Had some problems following this logic. But I think I got it now. What do you think about reformulating this (and the related comments further down) into
if (is_unsigned && lo >= 0) {
// cmp2 >= 0: val u<= cmp2 can only pass if val >= 0. Set val->_lo to 0.
} else {
// The lower bound of val cannot be improved.
lo = TypeInt::INT->_lo;
}
to make it more clear?
|
||
// Original (slightly simplified) fuzzer generated test | ||
public static void test1() { | ||
int i4, i5=99, i6, i9=89; |
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.
Minor: Add spaces
src/hotspot/share/opto/ifnode.cpp
Outdated
@@ -700,16 +720,17 @@ const TypeInt* IfNode::filtered_int_type(PhaseGVN* gvn, Node* val, Node* if_proj | |||
// | |||
|
|||
// Is the comparison for this If suitable for folding? | |||
bool IfNode::cmpi_folds(PhaseIterGVN* igvn, bool fold_ne) { | |||
bool IfNode::cmpi_folds(PhaseIterGVN* igvn) { |
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.
We should probably rename it into cmp_folds
now that it is also for unsigned and also update the description above. Shouldn't the description be at fold_compares()
anyways? But maybe we want to clean this up in a follow-up RFE.
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.
Yes, will rename cmpi_folds
to cmp_folds
.
Not changing the fold compares description location for now.
For now maybe it is okay location for description as that is starting of fold_compares implementation (all methods above fold_compares definition seems mainly its helper functions)
.e.g.: See the comment and declarations in [cfgnode.hpp]
........
private:
// Helper methods for fold_compares
bool cmp_folds(PhaseIterGVN* igvn);
bool is_ctrl_folds(Node* ctrl, PhaseIterGVN* igvn);
Or yes agree we can change this in a follow-up RFE.
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.
Yeah, it's probably best to leave it as it is at the moment and clean it up later in an RFE.
Thanks to Tobias and Christian for all help review comments, suggestions. |
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.
This looks good to me.
@r-v-raghav 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 134 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 |
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.
Looks good to me!
src/hotspot/share/opto/ifnode.cpp
Outdated
@@ -837,8 +857,94 @@ bool IfNode::has_only_uncommon_traps(ProjNode* proj, ProjNode*& success, ProjNod | |||
return false; | |||
} | |||
|
|||
// There might be an AddINode (marked with *) with a constant increment |
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.
Okay I see, thanks for the explanation.
src/hotspot/share/opto/ifnode.cpp
Outdated
} else { | ||
// The lower bound of val cannot be improved. | ||
lo = TypeInt::INT->_lo; | ||
} | ||
if (hi != min_jint) { | ||
hi = hi - 1; | ||
} | ||
break; |
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.
I think there's a bug here (case BoolTest::lt, unsigned comparison) . if cmp2 is [-1, 1] for instance, then lo and hi are unchanged. If the type of val is TypeInt::INT then the resulting type once join is applied is the interval [-1, 1]. But for instance 42 <u -1, 42 is in TypeInt::INT, -1 is in [-1, 1] but 42 is not in the resulting interval. I wonder if other cases in this method has similar issues. Can you fix that one and verify others?
This fixes the bug when the tests that need to be optimized out are next to one an another and the logic for folding comparisons can be used. It's hard to be certain that it's not possible for the 2 tests be separated by some other control flow. In that case the fix would fail. A conservative fix would be to not use a narrow type in the CastII that tableswitch/lookupswitch switch introduces. It would be a simpler fix and a more robust one. |
Thank you @rwestrel for the comments. i. Do proposal by Roland and remove the narrow type from the CastII as fix for this bug task. [src/hotspot/share/opto/parse2.cpp]
ii. Integrate the other proposed fix to improve |
Please note as per the above comments now updated the fix. Request help with review. Thanks. |
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.
This looks good to me. Please file a follow-up enhancement to integrate the optimization, I think it's still valuable.
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.
After quickly discussing this with @rwestrel, it turned out that you need to set CastIINode::_carry_dependency
to prevent the CastIINode
with TypeInt::INT
from being eliminated and re-introducing (JDK-6675699).
Thanks for the comments @TobiHartmann , @rwestrel . Please note now revised fix to add a Also created followup enhancement task - JDK-8263252. |
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 updating the change.
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.
Looks good to me too.
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.
Looks good!
Thanks to @rwestrel @TobiHartmann @chhagedorn for review approvals. (Now testing done - java_fuzzer cases / unit tests, mach5 tier 1-7 & pre-integration tests) |
/integrate |
@r-v-raghav Since your change was applied there have been 149 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 4b5be40. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Found that the earlier PR bug analysis and fix details done is wrong.
Request help to review this revised fix proposal.
Now understood need to fix two types of issues related to the bug-
(i). Crash with only
CmpI
nodes case.Sample test case -
// This generates the following subgraph:
Sample IR extracts -
Fix proposal here includes :
CmpU
related optimization out of oldfold_compares_helper()
,(new
fold_dominated_if()
added and renamed oldfold_compares_helper
asfold_to_unsigned()
).fold_compares()
and related code adjustments.(ii).
fold_compares()
not handlingCmpU
nodes cases.Sample basic test case -
Sample IR extracts -
Proposed fix includes:
Enabling
Op_CmpU
support in -cmpi_folds()
,filtered_int_type()
- returns possible restrictive type for input value based on condition control flow for the if (supportedCmpU
for various condition control types).fold_dominated_if()
- checks if dominating if determines the result of current if and above example test case is a basic variant possibility where bothCmpNode
s compare a common value directly. (Node 77 Phi
in above sample). But as mentioned in the comments of new helper functionget_base_comparing_value()
, there are other possibilities where there might be anAddINode
with a constant increment present in between theCmpNode
s and the common value compared.(Sample IR extract with
AddI
in betweenCmpNode
s : )All these variants are handled in
get_base_comparing_value()
and related test cases added.fold_compares()
.Found no issues with this proposed fix for various unit tests, reported java_fuzzer case, tier1-7 and pre-integeration tests
Progress
Issue
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2758/head:pull/2758
$ git checkout pull/2758
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2758/head:pull/2758
$ git checkout pull/2758