JDK-8230382 : Clean up ConvI2L, CastII and CastLL::Ideal methods
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 14
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-08-30
  • Updated: 2022-09-19
  • Resolved: 2022-02-28
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 19
19 b12Fixed
Related Reports
Relates :  
JDK-8288981 - C2: Fix issues with skeleton predicates
Relates :  
JDK-8283466 - C2: missing skeleton predicates in peeled loop
Relates :  
JDK-8292507 - C2: assert(found) failed: block b is not in n's home loop or an ancestor of it
Relates :  
JDK-8283460 - C2: assert(!LCA_orig->dominates(pred_block) || early->dominates(pred_block)) failed: early is high enough
Relates :  
JDK-8291025 - Jtreg compiler/loopopts/TestUnreachableInnerLoop.java fails with MaxVectorSize=8
Relates :  
JDK-8286638 - C2: CmpU needs to do more precise over/underflow analysis
Description
The following code should be moved into the corresponding ::Value methods:
http://hg.openjdk.java.net/jdk/jdk/file/bfb76c34e5c5/src/hotspot/share/opto/convertnode.cpp#l273
http://hg.openjdk.java.net/jdk/jdk/file/bfb76c34e5c5/src/hotspot/share/opto/castnode.cpp#l234

The same applies to the CastLLNode added by JDK-8229496.
Comments
There have now been several bugs "uncovered" by this change. What often happens is this: A ConvI2L discovers that its range is impossible (it is constrained to be positive bc of memory access below it, and the input is negative). Before, these discoveries were sometimes not made, and the node plus its outputs would not be removed. You may be able to find that ConvI2LNode with this change diff --git a/src/hotspot/share/opto/convertnode.cpp b/src/hotspot/share/opto/convertnode.cpp index 9f471c0958d..94270067271 100644 --- a/src/hotspot/share/opto/convertnode.cpp +++ b/src/hotspot/share/opto/convertnode.cpp @@ -260,6 +260,8 @@ const Type* ConvI2LNode::Value(PhaseGVN* phase) const { // Join my declared type against my incoming type. tl = tl->filter(_type); if (!tl->isa_long()) { + tty->print("ConvI2L::Value not long: "); tl->dump(); + this->dump(); return tl; } const TypeLong* this_type = tl->is_long(); Patterns of bugs that I have seen before: The ConvI2L dies, and rips out a some data-nodes, and messes up a phi further down, killing the data-flow but not the control flow. That then leads to issues in the scheduling, asserts where the dominate structure is not as expected. Specific to the skeleton/parse predicates: There used to be a CastII node above the ConvI2L, which links to the range check. This range-check needs to be right before the current loop, otherwise it is possible that the current loop has a smaller range than the RC checks, the current loop could die and the RC did not notice it. That leads to dead data and still alive control. Again, that leads to bad dominate structure. Don't hesitate to reach out if you have questions.
19-09-2022
Changeset: 06cadb36 Author: Emanuel Peter <emanuel.peter@oracle.com> Committer: Tobias Hartmann <thartmann@openjdk.org> Date: 2022-02-28 12:13:35 +0000 URL: https://git.openjdk.java.net/jdk/commit/06cadb36e05a86a528c8f3bc64c1a42b47ca94a0
28-02-2022
Additional asserts to refactor code a little more. Ran some tests to see that none of the asserts trigger. Now I can clean up the code accordingly. -------- diff --git a/src/hotspot/share/opto/castnode.cpp b/src/hotspot/share/opto/castnode.cpp index 1853a434eac..4ff410d25ac 100644 --- a/src/hotspot/share/opto/castnode.cpp +++ b/src/hotspot/share/opto/castnode.cpp @@ -194,7 +194,7 @@ void ConstraintCastNode::dump_spec(outputStream *st) const { const Type* CastIINode::Value(PhaseGVN* phase) const { const Type *res = ConstraintCastNode::Value(phase); - if(res == Type::TOP) { return res; } + if(res == Type::TOP) return Type::TOP; assert(res->isa_int(), "res must be int"); // Similar to ConvI2LNode::Value() for the same reasons @@ -203,31 +203,28 @@ const Type* CastIINode::Value(PhaseGVN* phase) const { // Do not narrow the type of range check dependent CastIINodes to // avoid corruption of the graph if a CastII is replaced by TOP but // the corresponding range check is not removed. - if (!_range_check_dependency) { - if (phase->C->post_loop_opts_phase()) { - const TypeInt* this_type = res->is_int(); - const TypeInt* in_type = phase->type(in(1))->isa_int(); - if (in_type != NULL && this_type != NULL && - (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { - jint lo1 = this_type->_lo; - jint hi1 = this_type->_hi; - int w1 = this_type->_widen; - - if (lo1 >= 0) { - // Keep a range assertion of >=0. - lo1 = 0; hi1 = max_jint; - } else if (hi1 < 0) { - // Keep a range assertion of <0. - lo1 = min_jint; hi1 = -1; - } else { - lo1 = min_jint; hi1 = max_jint; - } - const TypeInt* wtype = TypeInt::make(MAX2(in_type->_lo, lo1), - MIN2(in_type->_hi, hi1), - MAX2((int)in_type->_widen, w1)); - res = wtype; + if (!_range_check_dependency && phase->C->post_loop_opts_phase()) { + const TypeInt* this_type = res->is_int(); + const TypeInt* in_type = phase->type(in(1))->isa_int(); + if (in_type != NULL && + (in_type->_lo != this_type->_lo || + in_type->_hi != this_type->_hi)) { + jint lo1 = this_type->_lo; + jint hi1 = this_type->_hi; + int w1 = this_type->_widen; + + if (lo1 >= 0) { + // Keep a range assertion of >=0. + lo1 = 0; hi1 = max_jint; + } else if (hi1 < 0) { + // Keep a range assertion of <0. + lo1 = min_jint; hi1 = -1; + } else { + lo1 = min_jint; hi1 = max_jint; } + res = TypeInt::make(MAX2(in_type->_lo, lo1), + MIN2(in_type->_hi, hi1), + MAX2((int)in_type->_widen, w1)); } } @@ -309,7 +306,10 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { if (progress != NULL) { return progress; } - + if (can_reshape && !_range_check_dependency && !phase->C->post_loop_opts_phase()) { + // makes sure we run ::Value to potentially remove type assertion after loop opts + phase->C->record_for_post_loop_opts_igvn(this); + } PhaseIterGVN *igvn = phase->is_IterGVN(); const TypeInt* this_type = this->type()->is_int(); Node* z = in(1); @@ -335,12 +335,6 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { default: ShouldNotReachHere(); } } - if (can_reshape && !_range_check_dependency) { - if (!phase->C->post_loop_opts_phase()) { - // makes sure we run ::Value to potentially remove type assertion after loop opts - phase->C->record_for_post_loop_opts_igvn(this); - } - } return NULL; } diff --git a/src/hotspot/share/opto/convertnode.cpp b/src/hotspot/share/opto/convertnode.cpp index 246f2c3dcb6..139a1977ad9 100644 --- a/src/hotspot/share/opto/convertnode.cpp +++ b/src/hotspot/share/opto/convertnode.cpp @@ -259,44 +259,46 @@ const Type* ConvI2LNode::Value(PhaseGVN* phase) const { tl = tl->filter(_type); if(!tl->isa_long()) return tl; const TypeLong* this_type = tl->is_long(); - PhaseIterGVN *igvn = phase->is_IterGVN(); - if (igvn != NULL) { - // Do NOT remove this node's type assertion until no more loop ops can happen. - if (phase->C->post_loop_opts_phase()) { - const TypeInt* in_type = phase->type(in(1))->isa_int(); - if (in_type != NULL && this_type != NULL && - (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { - // Although this WORSENS the type, it increases GVN opportunities, - // because I2L nodes with the same input will common up, regardless - // of slightly differing type assertions. Such slight differences - // arise routinely as a result of loop unrolling, so this is a - // post-unrolling graph cleanup. Choose a type which depends only - // on my input. (Exception: Keep a range assertion of >=0 or <0.) - jlong lo1 = this_type->_lo; - jlong hi1 = this_type->_hi; - assert(lo1 <= hi1, "valid Long range"); - assert(in_type->_lo <= in_type->_hi, "valid Int range"); - int w1 = this_type->_widen; - if (lo1 != (jint)lo1 || - hi1 != (jint)hi1 || - lo1 > hi1) { - // Overflow leads to wraparound, wraparound leads to range saturation. - lo1 = min_jint; hi1 = max_jint; - } else if (lo1 >= 0) { - // Keep a range assertion of >=0. - lo1 = 0; hi1 = max_jint; - } else if (hi1 < 0) { - // Keep a range assertion of <0. - lo1 = min_jint; hi1 = -1; - } else { - lo1 = min_jint; hi1 = max_jint; - } - const TypeLong* wtype = TypeLong::make(MAX2((jlong)in_type->_lo, lo1), - MIN2((jlong)in_type->_hi, hi1), - MAX2((int)in_type->_widen, w1)); - return wtype; + PhaseIterGVN* igvn = phase->is_IterGVN(); + assert(!phase->C->post_loop_opts_phase() || igvn != NULL, "IGVN is present"); + // TODO: remove igvn + // Do NOT remove this node's type assertion until no more loop ops can happen. + if (igvn != NULL && phase->C->post_loop_opts_phase()) { + const TypeInt* in_type = phase->type(in(1))->isa_int(); + if (in_type != NULL && this_type != NULL && + (in_type->_lo != this_type->_lo || + in_type->_hi != this_type->_hi)) { + // Although this WORSENS the type, it increases GVN opportunities, + // because I2L nodes with the same input will common up, regardless + // of slightly differing type assertions. Such slight differences + // arise routinely as a result of loop unrolling, so this is a + // post-unrolling graph cleanup. Choose a type which depends only + // on my input. (Exception: Keep a range assertion of >=0 or <0.) + jlong lo1 = this_type->_lo; + jlong hi1 = this_type->_hi; + int w1 = this_type->_widen; + if (lo1 != (jint)lo1 || + hi1 != (jint)hi1 || + lo1 > hi1) { + // Overflow leads to wraparound, wraparound leads to range saturation. + lo1 = min_jint; hi1 = max_jint; +#ifdef ASSERT + fprintf(stderr, "this_type: %s\n", Type::str(this_type)); + assert(false, "I don't think this should happen"); + // TODO: remove this case +#endif + } else if (lo1 >= 0) { + // Keep a range assertion of >=0. + lo1 = 0; hi1 = max_jint; + } else if (hi1 < 0) { + // Keep a range assertion of <0. + lo1 = min_jint; hi1 = -1; + } else { + lo1 = min_jint; hi1 = max_jint; } + return TypeLong::make(MAX2((jlong)in_type->_lo, lo1), + MIN2((jlong)in_type->_hi, hi1), + MAX2((int)in_type->_widen, w1)); } } return this_type; @@ -403,7 +405,8 @@ bool Compile::push_thru_add(PhaseGVN* phase, Node* z, const TypeInteger* tz, con Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { PhaseIterGVN *igvn = phase->is_IterGVN(); const TypeLong* this_type = this->type()->is_long(); - + assert((igvn != NULL) == can_reshape, "IGVN precence assumption"); + // TODO: replace igvn with can_reshape if (igvn != NULL && !phase->C->post_loop_opts_phase()) { // makes sure we run ::Value to potentially remove type assertion after loop opts phase->C->record_for_post_loop_opts_igvn(this);
24-02-2022
A pull request was submitted for review. URL: https://git.openjdk.java.net/jdk/pull/7609 Date: 2022-02-24 13:03:31 +0000
24-02-2022
Intermediate result for CastIINode::Ideal and ::Value. I copied the code over to ::Value. Call it from ::Ideal to check if it gives the same results. In ::Value I return directly from the copied code. Afterwards I will just pass it on to later parts in ::Value. Ran test to see that assert does not trigger and no test fails. Note: I added "!this_type->empty()" to the original code if-statement. It would be dumb to not open up the range again if it is already going to be TOP once ::Value does the filtering. The copied code already knows that the filtered range cannot be empty. ----------- diff --git a/src/hotspot/share/opto/castnode.cpp b/src/hotspot/share/opto/castnode.cpp index 44799635db0..408e1f587c1 100644 --- a/src/hotspot/share/opto/castnode.cpp +++ b/src/hotspot/share/opto/castnode.cpp @@ -194,6 +194,51 @@ void ConstraintCastNode::dump_spec(outputStream *st) const { const Type* CastIINode::Value(PhaseGVN* phase) const { const Type *res = ConstraintCastNode::Value(phase); + if(res == Type::TOP) { return res; } + if(!res->isa_int()) { + fprintf(stderr, "res: %s\n",Type::str(res)); + } + assert(res->isa_int(), "res must be int"); + // Similar to ConvI2LNode::Value() for the same reasons + // see if we can remove type assertion after loop opts + // But here we have to pay extra attention: + // Do not narrow the type of range check dependent CastIINodes to + // avoid corruption of the graph if a CastII is replaced by TOP but + // the corresponding range check is not removed. + if (!_range_check_dependency) { + if (phase->C->post_loop_opts_phase()) { + const TypeInt* this_type = res->is_int(); + const TypeInt* in_type = phase->type(in(1))->isa_int(); + if (in_type != NULL && this_type != NULL && this_type != type() && + (in_type->_lo == this_type->_lo && + in_type->_hi == this_type->_hi)) { + // great, we have changed the type in the filter, + // and thereby converged to the input type + return this_type; + } else if (in_type != NULL && this_type != NULL && + (in_type->_lo != this_type->_lo || + in_type->_hi != this_type->_hi)) { + jint lo1 = this_type->_lo; + jint hi1 = this_type->_hi; + int w1 = this_type->_widen; + + if (lo1 >= 0) { + // Keep a range assertion of >=0. + lo1 = 0; hi1 = max_jint; + } else if (hi1 < 0) { + // Keep a range assertion of <0. + lo1 = min_jint; hi1 = -1; + } else { + lo1 = min_jint; hi1 = max_jint; + } + const TypeInt* wtype = TypeInt::make(MAX2(in_type->_lo, lo1), + MIN2(in_type->_hi, hi1), + MAX2((int)in_type->_widen, w1)); + //fprintf(stderr, "x res: %s\n",Type::str(res)); + return wtype; + } + } + } // Try to improve the type of the CastII if we recognize a CmpI/If // pattern. @@ -248,13 +293,14 @@ const Type* CastIINode::Value(PhaseGVN* phase) const { t = TypeInt::make(lo_int, hi_int, Type::WidenMax); res = res->filter_speculative(t); - + //fprintf(stderr, "z res: %s\n",Type::str(res)); return res; } } } } } + //fprintf(stderr, "y res: %s\n",Type::str(res)); return res; } @@ -302,6 +348,7 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { } // Similar to ConvI2LNode::Ideal() for the same reasons + // But other than in ConvI2LNode: // Do not narrow the type of range check dependent CastIINodes to // avoid corruption of the graph if a CastII is replaced by TOP but // the corresponding range check is not removed. @@ -311,7 +358,8 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { const TypeInt* in_type = phase->type(in(1))->isa_int(); if (in_type != NULL && this_type != NULL && (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { + in_type->_hi != this_type->_hi) && + !this_type->empty()) { // if range is empty rather wait for ::Value to clean it up jint lo1 = this_type->_lo; jint hi1 = this_type->_hi; int w1 = this_type->_widen; @@ -328,12 +376,25 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { const TypeInt* wtype = TypeInt::make(MAX2(in_type->_lo, lo1), MIN2(in_type->_hi, hi1), MAX2((int)in_type->_widen, w1)); + const Type* vtype = Value(phase); + if(!( (vtype->isa_int() && vtype->isa_int()->_lo == wtype->_lo && vtype->isa_int()->_hi == wtype->_hi) + ^ (wtype->empty() && vtype->empty()) )) { + fprintf(stderr, "vtype: %s\n",Type::str(vtype)); + fprintf(stderr, "wtype: %s\n",Type::str(wtype)); + fprintf(stderr, "in_type: %s\n",Type::str(in_type)); + fprintf(stderr, "this_type: %s\n",Type::str(this_type)); + const Type *res = ConstraintCastNode::Value(phase); + fprintf(stderr, "res: %s\n",Type::str(res)); + } + assert((vtype->isa_int() && vtype->isa_int()->_lo == wtype->_lo && vtype->isa_int()->_hi == wtype->_hi) + ^ (wtype->empty() && vtype->empty()), "refactoring assumption"); if (wtype != type()) { set_type(wtype); return this; } } } else { + // makes sure we run ::Value to potentially remove type assertion after loop opts phase->C->record_for_post_loop_opts_igvn(this); } }
24-02-2022
And this is a cleaned up version for CastIINode ::Ideal and ::Value: Ran larger test to ensure I didn't break anything. -------------- diff --git a/src/hotspot/share/opto/castnode.cpp b/src/hotspot/share/opto/castnode.cpp index 44799635db0..1853a434eac 100644 --- a/src/hotspot/share/opto/castnode.cpp +++ b/src/hotspot/share/opto/castnode.cpp @@ -194,6 +194,42 @@ void ConstraintCastNode::dump_spec(outputStream *st) const { const Type* CastIINode::Value(PhaseGVN* phase) const { const Type *res = ConstraintCastNode::Value(phase); + if(res == Type::TOP) { return res; } + assert(res->isa_int(), "res must be int"); + + // Similar to ConvI2LNode::Value() for the same reasons + // see if we can remove type assertion after loop opts + // But here we have to pay extra attention: + // Do not narrow the type of range check dependent CastIINodes to + // avoid corruption of the graph if a CastII is replaced by TOP but + // the corresponding range check is not removed. + if (!_range_check_dependency) { + if (phase->C->post_loop_opts_phase()) { + const TypeInt* this_type = res->is_int(); + const TypeInt* in_type = phase->type(in(1))->isa_int(); + if (in_type != NULL && this_type != NULL && + (in_type->_lo != this_type->_lo || + in_type->_hi != this_type->_hi)) { + jint lo1 = this_type->_lo; + jint hi1 = this_type->_hi; + int w1 = this_type->_widen; + + if (lo1 >= 0) { + // Keep a range assertion of >=0. + lo1 = 0; hi1 = max_jint; + } else if (hi1 < 0) { + // Keep a range assertion of <0. + lo1 = min_jint; hi1 = -1; + } else { + lo1 = min_jint; hi1 = max_jint; + } + const TypeInt* wtype = TypeInt::make(MAX2(in_type->_lo, lo1), + MIN2(in_type->_hi, hi1), + MAX2((int)in_type->_widen, w1)); + res = wtype; + } + } + } // Try to improve the type of the CastII if we recognize a CmpI/If // pattern. @@ -248,7 +284,6 @@ const Type* CastIINode::Value(PhaseGVN* phase) const { t = TypeInt::make(lo_int, hi_int, Type::WidenMax); res = res->filter_speculative(t); - return res; } } @@ -300,40 +335,9 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { default: ShouldNotReachHere(); } } - - // Similar to ConvI2LNode::Ideal() for the same reasons - // Do not narrow the type of range check dependent CastIINodes to - // avoid corruption of the graph if a CastII is replaced by TOP but - // the corresponding range check is not removed. if (can_reshape && !_range_check_dependency) { - if (phase->C->post_loop_opts_phase()) { - const TypeInt* this_type = this->type()->is_int(); - const TypeInt* in_type = phase->type(in(1))->isa_int(); - if (in_type != NULL && this_type != NULL && - (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { - jint lo1 = this_type->_lo; - jint hi1 = this_type->_hi; - int w1 = this_type->_widen; - - if (lo1 >= 0) { - // Keep a range assertion of >=0. - lo1 = 0; hi1 = max_jint; - } else if (hi1 < 0) { - // Keep a range assertion of <0. - lo1 = min_jint; hi1 = -1; - } else { - lo1 = min_jint; hi1 = max_jint; - } - const TypeInt* wtype = TypeInt::make(MAX2(in_type->_lo, lo1), - MIN2(in_type->_hi, hi1), - MAX2((int)in_type->_widen, w1)); - if (wtype != type()) { - set_type(wtype); - return this; - } - } - } else { + if (!phase->C->post_loop_opts_phase()) { + // makes sure we run ::Value to potentially remove type assertion after loop opts phase->C->record_for_post_loop_opts_igvn(this); } }
24-02-2022
Finished moving Code to ConvI2L::Value: Running larger tests now... -------------- diff --git a/src/hotspot/share/opto/convertnode.cpp b/src/hotspot/share/opto/convertnode.cpp index 501cb08c452..246f2c3dcb6 100644 --- a/src/hotspot/share/opto/convertnode.cpp +++ b/src/hotspot/share/opto/convertnode.cpp @@ -257,7 +257,49 @@ const Type* ConvI2LNode::Value(PhaseGVN* phase) const { const Type* tl = TypeLong::make(ti->_lo, ti->_hi, ti->_widen); // Join my declared type against my incoming type. tl = tl->filter(_type); - return tl; + if(!tl->isa_long()) return tl; + const TypeLong* this_type = tl->is_long(); + PhaseIterGVN *igvn = phase->is_IterGVN(); + if (igvn != NULL) { + // Do NOT remove this node's type assertion until no more loop ops can happen. + if (phase->C->post_loop_opts_phase()) { + const TypeInt* in_type = phase->type(in(1))->isa_int(); + if (in_type != NULL && this_type != NULL && + (in_type->_lo != this_type->_lo || + in_type->_hi != this_type->_hi)) { + // Although this WORSENS the type, it increases GVN opportunities, + // because I2L nodes with the same input will common up, regardless + // of slightly differing type assertions. Such slight differences + // arise routinely as a result of loop unrolling, so this is a + // post-unrolling graph cleanup. Choose a type which depends only + // on my input. (Exception: Keep a range assertion of >=0 or <0.) + jlong lo1 = this_type->_lo; + jlong hi1 = this_type->_hi; + assert(lo1 <= hi1, "valid Long range"); + assert(in_type->_lo <= in_type->_hi, "valid Int range"); + int w1 = this_type->_widen; + if (lo1 != (jint)lo1 || + hi1 != (jint)hi1 || + lo1 > hi1) { + // Overflow leads to wraparound, wraparound leads to range saturation. + lo1 = min_jint; hi1 = max_jint; + } else if (lo1 >= 0) { + // Keep a range assertion of >=0. + lo1 = 0; hi1 = max_jint; + } else if (hi1 < 0) { + // Keep a range assertion of <0. + lo1 = min_jint; hi1 = -1; + } else { + lo1 = min_jint; hi1 = max_jint; + } + const TypeLong* wtype = TypeLong::make(MAX2((jlong)in_type->_lo, lo1), + MIN2((jlong)in_type->_hi, hi1), + MAX2((int)in_type->_widen, w1)); + return wtype; + } + } + } + return this_type; } static inline bool long_ranges_overlap(jlong lo1, jlong hi1, @@ -361,50 +403,10 @@ bool Compile::push_thru_add(PhaseGVN* phase, Node* z, const TypeInteger* tz, con Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { PhaseIterGVN *igvn = phase->is_IterGVN(); const TypeLong* this_type = this->type()->is_long(); - Node* this_changed = NULL; - if (igvn != NULL) { - // Do NOT remove this node's type assertion until no more loop ops can happen. - if (phase->C->post_loop_opts_phase()) { - const TypeInt* in_type = phase->type(in(1))->isa_int(); - if (in_type != NULL && this_type != NULL && - (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { - // Although this WORSENS the type, it increases GVN opportunities, - // because I2L nodes with the same input will common up, regardless - // of slightly differing type assertions. Such slight differences - // arise routinely as a result of loop unrolling, so this is a - // post-unrolling graph cleanup. Choose a type which depends only - // on my input. (Exception: Keep a range assertion of >=0 or <0.) - jlong lo1 = this_type->_lo; - jlong hi1 = this_type->_hi; - int w1 = this_type->_widen; - if (lo1 != (jint)lo1 || - hi1 != (jint)hi1 || - lo1 > hi1) { - // Overflow leads to wraparound, wraparound leads to range saturation. - lo1 = min_jint; hi1 = max_jint; - } else if (lo1 >= 0) { - // Keep a range assertion of >=0. - lo1 = 0; hi1 = max_jint; - } else if (hi1 < 0) { - // Keep a range assertion of <0. - lo1 = min_jint; hi1 = -1; - } else { - lo1 = min_jint; hi1 = max_jint; - } - const TypeLong* wtype = TypeLong::make(MAX2((jlong)in_type->_lo, lo1), - MIN2((jlong)in_type->_hi, hi1), - MAX2((int)in_type->_widen, w1)); - if (wtype != type()) { - set_type(wtype); - // Note: this_type still has old type value, for the logic below. - this_changed = this; - } - } - } else { - phase->C->record_for_post_loop_opts_igvn(this); - } + if (igvn != NULL && !phase->C->post_loop_opts_phase()) { + // makes sure we run ::Value to potentially remove type assertion after loop opts + phase->C->record_for_post_loop_opts_igvn(this); } #ifdef _LP64 // Convert ConvI2L(AddI(x, y)) to AddL(ConvI2L(x), ConvI2L(y)) @@ -437,7 +439,7 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { // Postpone this optimization to iterative GVN, where we can handle deep // AddI chains without an exponential number of recursive Ideal() calls. phase->record_for_igvn(this); - return this_changed; + return NULL; } int op = z->Opcode(); Node* x = z->in(1); @@ -453,7 +455,7 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { } #endif //_LP64 - return this_changed; + return NULL; } //=============================================================================
22-02-2022
Intermediate result: copied code from ConvI2LNode::Ideal to ::Value Assert that return from ::Value is equivalent Ran Test to see that assert is not triggered and no Test fails. Next step is to remove old code from ::Ideal -------------- diff --git a/src/hotspot/share/opto/convertnode.cpp b/src/hotspot/share/opto/convertnode.cpp index 501cb08c452..3e31cf26329 100644 --- a/src/hotspot/share/opto/convertnode.cpp +++ b/src/hotspot/share/opto/convertnode.cpp @@ -254,10 +254,55 @@ const Type* ConvI2LNode::Value(PhaseGVN* phase) const { const Type *t = phase->type( in(1) ); if( t == Type::TOP ) return Type::TOP; const TypeInt *ti = t->is_int(); - const Type* tl = TypeLong::make(ti->_lo, ti->_hi, ti->_widen); + const Type* tl = TypeLong::make(ti->_lo, ti->_hi, MAX2(ti->_widen, type()->is_long()->_widen)); // Join my declared type against my incoming type. tl = tl->filter(_type); - return tl; + if(!tl->isa_long()) { + return tl; + } + const TypeLong* this_type = tl->is_long(); + PhaseIterGVN *igvn = phase->is_IterGVN(); + if (igvn != NULL) { + // Do NOT remove this node's type assertion until no more loop ops can happen. + if (phase->C->post_loop_opts_phase()) { + const TypeInt* in_type = phase->type(in(1))->isa_int(); + if (in_type != NULL && this_type != NULL && + (in_type->_lo != this_type->_lo || + in_type->_hi != this_type->_hi)) { + // Although this WORSENS the type, it increases GVN opportunities, + // because I2L nodes with the same input will common up, regardless + // of slightly differing type assertions. Such slight differences + // arise routinely as a result of loop unrolling, so this is a + // post-unrolling graph cleanup. Choose a type which depends only + // on my input. (Exception: Keep a range assertion of >=0 or <0.) + jlong lo1 = this_type->_lo; + jlong hi1 = this_type->_hi; + assert(lo1 <= hi1, "valid Long range"); + assert(in_type->_lo <= in_type->_hi, "valid Int range"); + int w1 = this_type->_widen; + if (lo1 != (jint)lo1 || + hi1 != (jint)hi1 || + lo1 > hi1) { + // Overflow leads to wraparound, wraparound leads to range saturation. + lo1 = min_jint; hi1 = max_jint; + } else if (lo1 >= 0) { + // Keep a range assertion of >=0. + lo1 = 0; hi1 = max_jint; + } else if (hi1 < 0) { + // Keep a range assertion of <0. + lo1 = min_jint; hi1 = -1; + } else { + lo1 = min_jint; hi1 = max_jint; + } + const TypeLong* wtype = TypeLong::make(MAX2((jlong)in_type->_lo, lo1), + MIN2((jlong)in_type->_hi, hi1), + MAX2((int)in_type->_widen, w1)); + // TODO: check if range valid? Or will this happen in a next ::Value iteration? + return wtype; + } + } + } + return this_type; } static inline bool long_ranges_overlap(jlong lo1, jlong hi1, @@ -367,9 +412,12 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { // Do NOT remove this node's type assertion until no more loop ops can happen. if (phase->C->post_loop_opts_phase()) { const TypeInt* in_type = phase->type(in(1))->isa_int(); + // Check that ranges not identical + // Check that Long is valid (lo<=hi), else wait for ::Value to make it TOP if (in_type != NULL && this_type != NULL && (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { + in_type->_hi != this_type->_hi) && + this_type->_lo <= this_type->_hi) { // Although this WORSENS the type, it increases GVN opportunities, // because I2L nodes with the same input will common up, regardless // of slightly differing type assertions. Such slight differences @@ -378,6 +426,8 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { // on my input. (Exception: Keep a range assertion of >=0 or <0.) jlong lo1 = this_type->_lo; jlong hi1 = this_type->_hi; + assert(lo1 <= hi1, "valid Long range"); + assert(in_type->_lo <= in_type->_hi, "valid Int range"); int w1 = this_type->_widen; if (lo1 != (jint)lo1 || hi1 != (jint)hi1 || @@ -396,6 +446,14 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { const TypeLong* wtype = TypeLong::make(MAX2((jlong)in_type->_lo, lo1), MIN2((jlong)in_type->_hi, hi1), MAX2((int)in_type->_widen, w1)); + /* debug */ const Type* vtype = Value(phase); +// if(!(vtype == wtype ^ (vtype == Type::TOP && wtype->_lo > wtype->_hi))) { +// fprintf(stderr, "vtype: %s\n", Type::str(vtype)); +// fprintf(stderr, "wtype: %s\n", Type::str(wtype)); +// fprintf(stderr, "this_type: %s\n", Type::str(this_type)); +// fprintf(stderr, "in_type: %s\n", Type::str(in_type)); +// } + /* debug */ assert((vtype == wtype) ^ ((vtype == Type::TOP) && (wtype->_lo > wtype->_hi)), "refactoring assumption"); if (wtype != type()) { set_type(wtype); // Note: this_type still has old type value, for the logic below. @@ -403,6 +461,7 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { } } } else { + // makes sure we run ::Value to potentially remove type assertion after loop opts phase->C->record_for_post_loop_opts_igvn(this); } }
22-02-2022
In some cases, we encounter a ConvI2LNode with a LongType of range 0..-1. This is an invalid range that would be eliminated in ::Value to TOP. The current code thinks this is an overflow case which leads to saturation, and extends the range. We should avoid this, and wait for ::Value to do its job. Replacing with TOP is ok, because unlike in CastLLNode, we do not have dependency with a RangeCheck. diff --git a/src/hotspot/share/opto/convertnode.cpp b/src/hotspot/share/opto/convertnode.cpp index 501cb08c452..4923f8d2b3a 100644 --- a/src/hotspot/share/opto/convertnode.cpp +++ b/src/hotspot/share/opto/convertnode.cpp @@ -367,9 +367,12 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { // Do NOT remove this node's type assertion until no more loop ops can happen. if (phase->C->post_loop_opts_phase()) { const TypeInt* in_type = phase->type(in(1))->isa_int(); + // Check that ranges not identical + // Check that Long is valid (lo<=hi), else wait for ::Value to make it TOP if (in_type != NULL && this_type != NULL && (in_type->_lo != this_type->_lo || - in_type->_hi != this_type->_hi)) { + in_type->_hi != this_type->_hi) && + this_type->_lo <= this_type->_hi) { // Although this WORSENS the type, it increases GVN opportunities, // because I2L nodes with the same input will common up, regardless // of slightly differing type assertions. Such slight differences @@ -379,6 +382,9 @@ Node *ConvI2LNode::Ideal(PhaseGVN *phase, bool can_reshape) { jlong lo1 = this_type->_lo; jlong hi1 = this_type->_hi; int w1 = this_type->_widen; + assert(lo1 <= hi1, "Long is valid"); + assert(in_type->_lo <= in_type->_hi, "Int is valid"); if (lo1 != (jint)lo1 || hi1 != (jint)hi1 || lo1 > hi1) {
22-02-2022
CastLLNode::Ideal was introduced in JDK-8229496: SIGFPE (division by zero) in C2 OSR compiled method and reverted (deleted) in JDK-8242108: Performance regression after fix for JDK-8229496 Currently, CastLLNode does not have its own ::Ideal method.
19-02-2022
[~azeemj] can we re-assign this or do you plan to work on it?
07-02-2022