From c9a7b9772d96d9a4825d9da2aacc277534282860 Mon Sep 17 00:00:00 2001 From: Roland Westrelin Date: Thu, 23 May 2024 16:37:01 +0000 Subject: [PATCH] 8332829: [BACKOUT] C2: crash in compiled code because of dependency on removed range check CastIIs Reviewed-by: thartmann --- src/hotspot/share/opto/castnode.cpp | 19 +- src/hotspot/share/opto/compile.cpp | 50 +- src/hotspot/share/opto/compile.hpp | 3 - src/hotspot/share/opto/node.cpp | 12 - src/hotspot/share/opto/node.hpp | 7 - ...yAccessAboveRCAfterRCCastIIEliminated.java | 474 ------------------ 6 files changed, 26 insertions(+), 539 deletions(-) delete mode 100644 test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterRCCastIIEliminated.java diff --git a/src/hotspot/share/opto/castnode.cpp b/src/hotspot/share/opto/castnode.cpp index 1771107c851..a93c9b382f9 100644 --- a/src/hotspot/share/opto/castnode.cpp +++ b/src/hotspot/share/opto/castnode.cpp @@ -213,7 +213,13 @@ const Type* CastIINode::Value(PhaseGVN* phase) const { // Similar to ConvI2LNode::Value() for the same reasons // see if we can remove type assertion after loop opts - res = widen_type(phase, res, T_INT); + // 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) { + res = widen_type(phase, res, T_INT); + } return res; } @@ -233,11 +239,11 @@ Node *CastIINode::Ideal(PhaseGVN *phase, bool can_reshape) { if (progress != nullptr) { return progress; } - if (can_reshape && !phase->C->post_loop_opts_phase()) { + 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); } - if (!_type->is_int()->empty()) { + if (!_range_check_dependency) { return optimize_integer_cast(phase, T_INT); } return nullptr; @@ -248,6 +254,13 @@ Node* CastIINode::Identity(PhaseGVN* phase) { if (progress != this) { return progress; } + if (_range_check_dependency) { + if (phase->C->post_loop_opts_phase()) { + return this->in(1); + } else { + phase->C->record_for_post_loop_opts_igvn(this); + } + } return this; } diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp index 445f22c6b80..25c7ced8aeb 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -3464,10 +3464,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f } break; } - case Op_CastII: { - remove_range_check_cast(n->as_CastII()); - } - break; #ifdef _LP64 case Op_CmpP: // Do this transformation here to preserve CmpPNode::sub() and @@ -3619,6 +3615,16 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f #endif +#ifdef ASSERT + case Op_CastII: + // Verify that all range check dependent CastII nodes were removed. + if (n->isa_CastII()->has_range_check()) { + n->dump(3); + assert(false, "Range check dependent CastII node was not removed"); + } + break; +#endif + case Op_ModI: if (UseDivMod) { // Check if a%b and a/b both exist @@ -3627,8 +3633,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f // Replace them with a fused divmod if supported if (Matcher::has_match_rule(Op_DivModI)) { DivModINode* divmod = DivModINode::make(n); - divmod->add_prec_from(n); - divmod->add_prec_from(d); d->subsume_by(divmod->div_proj(), this); n->subsume_by(divmod->mod_proj(), this); } else { @@ -3649,8 +3653,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f // Replace them with a fused divmod if supported if (Matcher::has_match_rule(Op_DivModL)) { DivModLNode* divmod = DivModLNode::make(n); - divmod->add_prec_from(n); - divmod->add_prec_from(d); d->subsume_by(divmod->div_proj(), this); n->subsume_by(divmod->mod_proj(), this); } else { @@ -3671,8 +3673,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f // Replace them with a fused unsigned divmod if supported if (Matcher::has_match_rule(Op_UDivModI)) { UDivModINode* divmod = UDivModINode::make(n); - divmod->add_prec_from(n); - divmod->add_prec_from(d); d->subsume_by(divmod->div_proj(), this); n->subsume_by(divmod->mod_proj(), this); } else { @@ -3693,8 +3693,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f // Replace them with a fused unsigned divmod if supported if (Matcher::has_match_rule(Op_UDivModL)) { UDivModLNode* divmod = UDivModLNode::make(n); - divmod->add_prec_from(n); - divmod->add_prec_from(d); d->subsume_by(divmod->div_proj(), this); n->subsume_by(divmod->mod_proj(), this); } else { @@ -3896,34 +3894,6 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f } } -void Compile::remove_range_check_cast(CastIINode* cast) { - if (cast->has_range_check()) { - // Range check CastII nodes feed into an address computation subgraph. Remove them to let that subgraph float freely. - // For memory access or integer divisions nodes that depend on the cast, record the dependency on the cast's control - // as a precedence edge, so they can't float above the cast in case that cast's narrowed type helped eliminate a - // range check or a null divisor check. - assert(cast->in(0) != nullptr, "All RangeCheck CastII must have a control dependency"); - ResourceMark rm; - Unique_Node_List wq; - wq.push(cast); - for (uint next = 0; next < wq.size(); ++next) { - Node* m = wq.at(next); - for (DUIterator_Fast imax, i = m->fast_outs(imax); i < imax; i++) { - Node* use = m->fast_out(i); - if (use->is_Mem() || use->is_div_or_mod(T_INT) || use->is_div_or_mod(T_LONG)) { - use->ensure_control_or_add_prec(cast->in(0)); - } else if (!use->is_CFG() && !use->is_Phi()) { - wq.push(use); - } - } - } - cast->subsume_by(cast->in(1), this); - if (cast->outcnt() == 0) { - cast->disconnect_inputs(this); - } - } -} - //------------------------------final_graph_reshaping_walk--------------------- // Replacing Opaque nodes with their input in final_graph_reshaping_impl(), // requires that the walk visits a node's inputs before visiting the node. diff --git a/src/hotspot/share/opto/compile.hpp b/src/hotspot/share/opto/compile.hpp index 9a873b8b9cb..e1d9b61f7f8 100644 --- a/src/hotspot/share/opto/compile.hpp +++ b/src/hotspot/share/opto/compile.hpp @@ -53,7 +53,6 @@ class Block; class Bundle; class CallGenerator; class CallStaticJavaNode; -class CastIINode; class CloneMap; class CompilationFailureInfo; class ConnectionGraph; @@ -1315,8 +1314,6 @@ private: BasicType out_bt, BasicType in_bt); static Node* narrow_value(BasicType bt, Node* value, const Type* type, PhaseGVN* phase, bool transform_res); - - void remove_range_check_cast(CastIINode* cast); }; #endif // SHARE_OPTO_COMPILE_HPP diff --git a/src/hotspot/share/opto/node.cpp b/src/hotspot/share/opto/node.cpp index cadea46e2c9..fa85344d0da 100644 --- a/src/hotspot/share/opto/node.cpp +++ b/src/hotspot/share/opto/node.cpp @@ -2878,15 +2878,6 @@ void Node::ensure_control_or_add_prec(Node* c) { } } -void Node::add_prec_from(Node* n) { - for (uint i = n->req(); i < n->len(); i++) { - Node* prec = n->in(i); - if (prec != nullptr) { - add_prec(prec); - } - } -} - bool Node::is_dead_loop_safe() const { if (is_Phi()) { return true; @@ -2910,9 +2901,6 @@ bool Node::is_dead_loop_safe() const { return false; } -bool Node::is_div_or_mod(BasicType bt) const { return Opcode() == Op_Div(bt) || Opcode() == Op_Mod(bt) || - Opcode() == Op_UDiv(bt) || Opcode() == Op_UMod(bt); } - //============================================================================= //------------------------------yank------------------------------------------- // Find and remove diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 412e193a612..04631dcbae2 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -1143,7 +1143,6 @@ public: // Set control or add control as precedence edge void ensure_control_or_add_prec(Node* c); - void add_prec_from(Node* n); // Visit boundary uses of the node and apply a callback function for each. // Recursively traverse uses, stopping and applying the callback when @@ -1255,8 +1254,6 @@ public: // Whether this is a memory phi node bool is_memory_phi() const { return is_Phi() && bottom_type() == Type::MEMORY; } - bool is_div_or_mod(BasicType bt) const; - //----------------- Printing, etc #ifndef PRODUCT public: @@ -2026,10 +2023,6 @@ Op_IL(URShift) Op_IL(LShift) Op_IL(Xor) Op_IL(Cmp) -Op_IL(Div) -Op_IL(Mod) -Op_IL(UDiv) -Op_IL(UMod) inline int Op_ConIL(BasicType bt) { assert(bt == T_INT || bt == T_LONG, "only for int or longs"); diff --git a/test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterRCCastIIEliminated.java b/test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterRCCastIIEliminated.java deleted file mode 100644 index 7f22db08a36..00000000000 --- a/test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterRCCastIIEliminated.java +++ /dev/null @@ -1,474 +0,0 @@ -/* - * Copyright (c) 2024, Red Hat, Inc. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA - * or visit www.oracle.com if you need additional information or have any - * questions. - */ - -/** - * @test - * @bug 8324517 - * @summary C2: out of bound array load because of dependency on removed range check CastIIs - * - * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation - * -XX:CompileCommand=dontinline,TestArrayAccessAboveRCAfterRCCastIIEliminated::notInlined - * TestArrayAccessAboveRCAfterRCCastIIEliminated - * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation - * -XX:CompileCommand=dontinline,TestArrayAccessAboveRCAfterRCCastIIEliminated::notInlined - * -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM TestArrayAccessAboveRCAfterRCCastIIEliminated - * @run main/othervm TestArrayAccessAboveRCAfterRCCastIIEliminated - * @run main/othervm -XX:CompileCommand=dontinline,TestArrayAccessAboveRCAfterRCCastIIEliminated::notInlined - * TestArrayAccessAboveRCAfterRCCastIIEliminated - * - */ - -public class TestArrayAccessAboveRCAfterRCCastIIEliminated { - private static int intField; - private static long longField; - private static volatile int volatileField; - - public static void main(String[] args) { - int[] array = new int[100]; - for (int i = 0; i < 20_000; i++) { - test1(9, 10, 1, true); - test1(9, 10, 1, false); - test2(9, 10, 1, true); - test2(9, 10, 1, false); - test3(9, 10, 1, true); - test3(9, 10, 1, false); - test4(9, 10, 1, true); - test4(9, 10, 1, false); - test5(9, 10, 1, true); - test5(9, 10, 1, false); - test6(9, 10, 1, true); - test6(9, 10, 1, false); - test7(9, 10, 1, true); - test7(9, 10, 1, false); - test8(9, 10, 1, true); - test8(9, 10, 1, false); - test9(9, 10, 1, true); - test9(9, 10, 1, false); - test10(9, 10, 1, true); - test10(9, 10, 1, false); - test11(9, 10, 1, true); - test11(9, 10, 1, false); - test12(9, 10, 1, true); - test12(9, 10, 1, false); - test13(9, 10, 1, true); - test13(9, 10, 1, false); - } - try { - test1(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test2(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test3(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test4(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test5(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test6(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test7(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test8(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test9(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test10(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test11(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test12(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - try { - test13(-1, 10, 1, true); - } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) { - } - } - - private static void test1(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = array[otherArray.length]; - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = array[otherArray.length]; - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test2(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = 1 / (otherArray.length + 1); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = 1 / (otherArray.length + 1); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test3(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = 1L / (otherArray.length + 1); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = 1L / (otherArray.length + 1); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test4(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = 1 % (otherArray.length + 1); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = 1 % (otherArray.length + 1); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test5(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = 1L % (otherArray.length + 1); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = 1L % (otherArray.length + 1); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test6(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = 1 % (otherArray.length + 1) + 1 / (otherArray.length + 1); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = 1 % (otherArray.length + 1) + 1 / (otherArray.length + 1); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test7(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = 1L % (otherArray.length + 1) + 1L / (otherArray.length + 1); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = 1L % (otherArray.length + 1) + 1L / (otherArray.length + 1); - } - for (int k = 0; k < 10; k++) { - - } - } - private static void test8(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = Integer.divideUnsigned(1, (otherArray.length + 1)); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = Integer.divideUnsigned(1, (otherArray.length + 1)); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test9(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = Long.divideUnsigned(1L, (otherArray.length + 1)); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = Long.divideUnsigned(1L, (otherArray.length + 1)); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test10(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = Integer.remainderUnsigned(1, (otherArray.length + 1)); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = Integer.remainderUnsigned(1, (otherArray.length + 1)); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test11(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = Long.remainderUnsigned(1L, (otherArray.length + 1)); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = Long.remainderUnsigned(1L, (otherArray.length + 1)); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test12(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = Integer.divideUnsigned(1, (otherArray.length + 1)) + - Integer.remainderUnsigned(1, (otherArray.length + 1)); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - intField = Integer.divideUnsigned(1, (otherArray.length + 1)) + - Integer.remainderUnsigned(1, (otherArray.length + 1)); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void test13(int i, int j, int flag, boolean flag2) { - i = Math.min(i, 9); - int[] array = new int[10]; - notInlined(array); - if (flag == 0) { - } - if (flag2) { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = Long.remainderUnsigned(1L, (otherArray.length + 1)) + - Long.divideUnsigned(1L, (otherArray.length + 1)); - } else { - float[] newArray = new float[j]; - newArray[i] = 42; - float[] otherArray = new float[i]; - if (flag == 0) { - } - longField = Long.remainderUnsigned(1L, (otherArray.length + 1)) + - Long.divideUnsigned(1L, (otherArray.length + 1)); - } - for (int k = 0; k < 10; k++) { - - } - } - - private static void notInlined(int[] array) { - - } -}