diff --git a/src/hotspot/share/opto/addnode.hpp b/src/hotspot/share/opto/addnode.hpp index 28ed73121edca..1bbdae92e486a 100644 --- a/src/hotspot/share/opto/addnode.hpp +++ b/src/hotspot/share/opto/addnode.hpp @@ -246,6 +246,13 @@ class AddPNode : public Node { // Do not match base-ptr edge virtual uint match_edge(uint idx) const; + +#ifdef ASSERT + bool address_input_has_same_base() const { + Node *addp = in(Address); + return !addp->is_AddP() || addp->in(Base)->is_top() || addp->in(Base) == in(Base); + } +#endif }; //------------------------------OrINode---------------------------------------- diff --git a/src/hotspot/share/opto/c2_globals.hpp b/src/hotspot/share/opto/c2_globals.hpp index 2b2b4db47b134..7fa3ca638c2cd 100644 --- a/src/hotspot/share/opto/c2_globals.hpp +++ b/src/hotspot/share/opto/c2_globals.hpp @@ -697,7 +697,8 @@ "Print progress during Iterative Global Value Numbering") \ \ develop(uint, VerifyIterativeGVN, 0, \ - "Verify Iterative Global Value Numbering =DCBA, with:" \ + "Verify Iterative Global Value Numbering =EDCBA, with:" \ + " E: verify node specific invariants" \ " D: verify Node::Identity did not miss opportunities" \ " C: verify Node::Ideal did not miss opportunities" \ " B: verify that type(n) == n->Value() after IGVN" \ diff --git a/src/hotspot/share/opto/cfgnode.cpp b/src/hotspot/share/opto/cfgnode.cpp index 0598f10b880df..a6b8cd9db0a19 100644 --- a/src/hotspot/share/opto/cfgnode.cpp +++ b/src/hotspot/share/opto/cfgnode.cpp @@ -2096,6 +2096,20 @@ bool PhiNode::is_split_through_mergemem_terminating() const { return true; } +// Is one of the inputs a Cast that has not been processed by igvn yet? +bool PhiNode::wait_for_cast_input_igvn(const PhaseIterGVN* igvn) const { + for (uint i = 1, cnt = req(); i < cnt; ++i) { + Node* n = in(i); + while (n != nullptr && n->is_ConstraintCast()) { + if (igvn->_worklist.member(n)) { + return true; + } + n = n->in(1); + } + } + return false; +} + //------------------------------Ideal------------------------------------------ // Return a node which is more "ideal" than the current node. Must preserve // the CFG, but we can still strip out dead paths. @@ -2154,6 +2168,28 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) { // If there is a chance that the region can be optimized out do // not add a cast node that we can't remove yet. !wait_for_region_igvn(phase)) { + // If one of the inputs is a cast that has yet to be processed by igvn, delay processing of this node to give the + // inputs a chance to optimize and possibly end up with identical inputs (casts included). + // Say we have: + // (Phi region (Cast#1 c uin) (Cast#2 c uin)) + // and Cast#1 and Cast#2 have not had a chance to common yet + // if the unique_input() transformation below proceeds, then PhiNode::Ideal returns: + // (Cast#3 region uin) (1) + // If PhiNode::Ideal is delayed until Cast#1 and Cast#2 common, then it returns: + // (Cast#1 c uin) (2) + // + // In (1) the resulting cast is conservatively pinned at a later control and while Cast#3 and Cast#1/Cast#2 still + // have a chance to common, that requires proving that c dominates region in ConstraintCastNode::dominating_cast() + // which may not happen if control flow is too complicated and another pass of loop opts doesn't run. Delaying the + // transformation here should allow a more optimal result. + // Beyond the efficiency concern, there is a risk, if the casts are CastPPs, to end up with a chain of AddPs with + // different base inputs (but a unique uncasted base input). This breaks an invariant in the shape of address + // subtrees. + PhaseIterGVN* igvn = phase->is_IterGVN(); + if (wait_for_cast_input_igvn(igvn)) { + igvn->_worklist.push(this); + return nullptr; + } uncasted = true; uin = unique_input(phase, true); } diff --git a/src/hotspot/share/opto/cfgnode.hpp b/src/hotspot/share/opto/cfgnode.hpp index bc0b38e2f97ad..f3ccf23703f17 100644 --- a/src/hotspot/share/opto/cfgnode.hpp +++ b/src/hotspot/share/opto/cfgnode.hpp @@ -182,6 +182,8 @@ class PhiNode : public TypeNode { bool is_split_through_mergemem_terminating() const; + bool wait_for_cast_input_igvn(const PhaseIterGVN* igvn) const; + public: // Node layout (parallels RegionNode): enum { Region, // Control input is the Phi's region. diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp index a89b5b00a25d0..907a9592e0abd 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -3414,10 +3414,7 @@ void Compile::final_graph_reshaping_main_switch(Node* n, Final_Reshape_Counts& f case Op_AddP: { // Assert sane base pointers Node *addp = n->in(AddPNode::Address); - assert( !addp->is_AddP() || - addp->in(AddPNode::Base)->is_top() || // Top OK for allocation - addp->in(AddPNode::Base) == n->in(AddPNode::Base), - "Base pointers must match (addp %u)", addp->_idx ); + assert(n->as_AddP()->address_input_has_same_base(), "Base pointers must match (addp %u)", addp->_idx ); #ifdef _LP64 if ((UseCompressedOops || UseCompressedClassPointers) && addp->Opcode() == Op_ConP && diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 4a0933b89f29f..1e4cd42e09a05 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1076,7 +1076,8 @@ void PhaseIterGVN::verify_optimize() { if (is_verify_Value() || is_verify_Ideal() || - is_verify_Identity()) { + is_verify_Identity() || + is_verify_invariants()) { ResourceMark rm; Unique_Node_List worklist; bool failure = false; @@ -1088,6 +1089,7 @@ void PhaseIterGVN::verify_optimize() { if (is_verify_Ideal()) { failure |= verify_Ideal_for(n, false); } if (is_verify_Ideal()) { failure |= verify_Ideal_for(n, true); } if (is_verify_Identity()) { failure |= verify_Identity_for(n); } + if (is_verify_invariants()) { failure |= verify_node_invariants_for(n); } // traverse all inputs and outputs for (uint i = 0; i < n->req(); i++) { if (n->in(i) != nullptr) { @@ -1102,7 +1104,7 @@ void PhaseIterGVN::verify_optimize() { // We should either make sure that these nodes are properly added back to the IGVN worklist // in PhaseIterGVN::add_users_to_worklist to update them again or add an exception // in the verification code above if that is not possible for some reason (like Load nodes). - assert(!failure, "Missed optimization opportunity in PhaseIterGVN"); + assert(!failure, "Missed optimization opportunity/broken graph in PhaseIterGVN"); } verify_empty_worklist(nullptr); @@ -2058,6 +2060,21 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { tty->print_cr("%s", ss.as_string()); return true; } + +// Some other verifications that are not specific to a particular transformation. +bool PhaseIterGVN::verify_node_invariants_for(const Node* n) { + if (n->is_AddP()) { + if (!n->as_AddP()->address_input_has_same_base()) { + stringStream ss; // Print as a block without tty lock. + ss.cr(); + ss.print_cr("Base pointers must match for AddP chain:"); + n->dump_bfs(2, nullptr, "", &ss); + tty->print_cr("%s", ss.as_string()); + return true; + } + } + return false; +} #endif /** diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index 473231e6af5db..3f75aab898097 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -493,6 +493,7 @@ class PhaseIterGVN : public PhaseGVN { bool verify_Value_for(Node* n, bool strict = false); bool verify_Ideal_for(Node* n, bool can_reshape); bool verify_Identity_for(Node* n); + bool verify_node_invariants_for(const Node* n); void verify_empty_worklist(Node* n); #endif @@ -616,6 +617,10 @@ class PhaseIterGVN : public PhaseGVN { // '-XX:VerifyIterativeGVN=1000' return ((VerifyIterativeGVN % 10000) / 1000) == 1; } + static bool is_verify_invariants() { + // '-XX:VerifyIterativeGVN=10000' + return ((VerifyIterativeGVN % 100000) / 10000) == 1; + } protected: // Sub-quadratic implementation of '-XX:VerifyIterativeGVN=1' (Use-Def verification). julong _verify_counter; diff --git a/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp b/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp index d0141c2e6ccfc..63d3424b3edd9 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp @@ -306,7 +306,7 @@ JVMFlag::Error TypeProfileLevelConstraintFunc(uint value, bool verbose) { } JVMFlag::Error VerifyIterativeGVNConstraintFunc(uint value, bool verbose) { - const int max_modes = 4; + const int max_modes = 5; uint original_value = value; for (int i = 0; i < max_modes; i++) { if (value % 10 > 1) { diff --git a/test/hotspot/jtreg/compiler/c2/TestMismatchedAddPAfterMaxUnroll.java b/test/hotspot/jtreg/compiler/c2/TestMismatchedAddPAfterMaxUnroll.java new file mode 100644 index 0000000000000..2dcafd2cf6a57 --- /dev/null +++ b/test/hotspot/jtreg/compiler/c2/TestMismatchedAddPAfterMaxUnroll.java @@ -0,0 +1,80 @@ +/* + * Copyright (c) 2025, 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 8351889 + * @summary C2 crash: assertion failed: Base pointers must match (addp 344) + * @run main/othervm -XX:-BackgroundCompilation -XX:CompileOnly=TestMismatchedAddPAfterMaxUnroll::test1 + * -XX:+UnlockDiagnosticVMOptions -XX:+IgnoreUnrecognizedVMOptions -XX:-UseLoopPredicate + * -XX:+StressIGVN -XX:StressSeed=383593806 -XX:VerifyIterativeGVN=10000 + * TestMismatchedAddPAfterMaxUnroll + * @run main/othervm -XX:-BackgroundCompilation -XX:CompileOnly=TestMismatchedAddPAfterMaxUnroll::test1 + * -XX:+UnlockDiagnosticVMOptions -XX:+IgnoreUnrecognizedVMOptions -XX:-UseLoopPredicate + * -XX:+StressIGVN -XX:VerifyIterativeGVN=10000 TestMismatchedAddPAfterMaxUnroll + * @run main/othervm TestMismatchedAddPAfterMaxUnroll + */ + +public class TestMismatchedAddPAfterMaxUnroll { + private static C[] arrayField = new C[4]; + + public static void main(String[] args) { + C c = new C(); + Object lock = new Object(); + for (int i = 0; i < 20_000; i++) { + arrayField[3] = null; + test1(3, c, arrayField, true, true, lock); + arrayField[3] = null; + test1(3, c, arrayField, true, false, lock); + arrayField[3] = null; + test1(3, c, arrayField, false, false, lock); + arrayField[3] = c; + test1(3, c, arrayField, false, false, lock); + } + } + + static class C { + + } + + private static void test1(int j, C c, C[] otherArray, boolean flag, boolean flag2, Object lock) { + C[] array = arrayField; + int i = 0; + for (;;) { + synchronized (lock) {} + if (array[j] == null) { + break; + } + otherArray[i] = c; + i++; + if (i >= 3) { + return; + } + } + if (flag) { + if (flag2) { + } + } + array[j] = c; + } +} diff --git a/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java b/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java index 83f3540226f43..4b6215b25bd9e 100644 --- a/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java +++ b/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java @@ -23,11 +23,11 @@ /* * @test - * @bug 8238756 + * @bug 8238756 8351889 * @requires vm.debug == true & vm.flavor == "server" - * @summary Run with -Xcomp to test -XX:VerifyIterativeGVN=1111 in debug builds. + * @summary Run with -Xcomp to test -XX:VerifyIterativeGVN=11111 in debug builds. * - * @run main/othervm/timeout=300 -Xcomp -XX:VerifyIterativeGVN=1111 compiler.c2.TestVerifyIterativeGVN + * @run main/othervm/timeout=300 -Xcomp -XX:VerifyIterativeGVN=11111 compiler.c2.TestVerifyIterativeGVN */ package compiler.c2;