From ec30a8c75528cc96cf3357322b08f51ec2ebc842 Mon Sep 17 00:00:00 2001 From: Damon Fenacci Date: Thu, 9 Oct 2025 06:21:08 +0000 Subject: [PATCH 1/7] 8360031: C2 compilation asserts in MemBarNode::remove Reviewed-by: dlong, kvn, shade --- src/hotspot/share/opto/escape.cpp | 2 +- src/hotspot/share/opto/memnode.cpp | 5 +---- .../jtreg/compiler/c2/irTests/ConstructorBarriers.java | 5 ++++- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/hotspot/share/opto/escape.cpp b/src/hotspot/share/opto/escape.cpp index d7253b06599..780121ab087 100644 --- a/src/hotspot/share/opto/escape.cpp +++ b/src/hotspot/share/opto/escape.cpp @@ -201,7 +201,7 @@ bool ConnectionGraph::compute_escape() { if (!UseStoreStoreForCtor || n->req() > MemBarNode::Precedent) { storestore_worklist.append(n->as_MemBarStoreStore()); } - break; + // If MemBarStoreStore has a precedent edge add it to the worklist (like MemBarRelease) case Op_MemBarRelease: if (n->req() > MemBarNode::Precedent) { record_for_optimizer(n); diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp index 24b81b894cb..27565ea397e 100644 --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -4231,10 +4231,7 @@ MemBarNode* MemBarNode::make(Compile* C, int opcode, int atp, Node* pn) { } void MemBarNode::remove(PhaseIterGVN *igvn) { - if (outcnt() != 2) { - assert(Opcode() == Op_Initialize, "Only seen when there are no use of init memory"); - assert(outcnt() == 1, "Only control then"); - } + assert(outcnt() > 0 && outcnt() <= 2, "Only one or two out edges allowed"); if (trailing_store() || trailing_load_store()) { MemBarNode* leading = leading_membar(); if (leading != nullptr) { diff --git a/test/hotspot/jtreg/compiler/c2/irTests/ConstructorBarriers.java b/test/hotspot/jtreg/compiler/c2/irTests/ConstructorBarriers.java index 7252427ffcd..ba7e7d851b0 100644 --- a/test/hotspot/jtreg/compiler/c2/irTests/ConstructorBarriers.java +++ b/test/hotspot/jtreg/compiler/c2/irTests/ConstructorBarriers.java @@ -123,9 +123,12 @@ public VolatileVolatile(long i) { } long l = 42; + volatile Object global; @DontInline - public void consume(Object o) {} + public void consume(Object o) { + global = o; + } @Test @IR(counts = {IRNode.MEMBAR_STORESTORE, "1"}) From c9aee8cdff2ff13f157e0586356506765fd9f258 Mon Sep 17 00:00:00 2001 From: Roland Westrelin Date: Tue, 4 Nov 2025 11:17:56 +0000 Subject: [PATCH 2/7] 8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Emanuel Peter Co-authored-by: Roberto Castañeda Lozano Reviewed-by: epeter, rcastanedalo --- src/hotspot/share/opto/classes.hpp | 1 + src/hotspot/share/opto/escape.cpp | 52 ++- src/hotspot/share/opto/escape.hpp | 8 +- src/hotspot/share/opto/graphKit.cpp | 15 +- src/hotspot/share/opto/idealGraphPrinter.cpp | 2 +- src/hotspot/share/opto/library_call.cpp | 18 +- src/hotspot/share/opto/loopTransform.cpp | 4 +- src/hotspot/share/opto/loopopts.cpp | 5 +- src/hotspot/share/opto/macro.cpp | 49 ++- src/hotspot/share/opto/matcher.cpp | 22 +- src/hotspot/share/opto/memnode.cpp | 59 ++- src/hotspot/share/opto/memnode.hpp | 44 ++- src/hotspot/share/opto/multnode.cpp | 73 ++-- src/hotspot/share/opto/multnode.hpp | 134 +++++++ src/hotspot/share/opto/node.cpp | 2 +- src/hotspot/share/opto/node.hpp | 3 + src/hotspot/share/opto/phaseX.cpp | 10 +- src/hotspot/share/opto/stringopts.cpp | 8 +- .../ServerCompilerScheduler.java | 4 +- .../filters/condenseGraph.filter | 1 + .../escapeAnalysis/TestIterativeEA.java | 10 +- ...arlyEliminationOfAllocationWithoutUse.java | 67 ++++ ...TestEliminationOfAllocationWithoutUse.java | 338 ++++++++++++++++++ .../TestInitializingStoreCapturing.java | 59 +++ 24 files changed, 897 insertions(+), 91 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/macronodes/TestEarlyEliminationOfAllocationWithoutUse.java create mode 100644 test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java create mode 100644 test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java diff --git a/src/hotspot/share/opto/classes.hpp b/src/hotspot/share/opto/classes.hpp index bc259eed2d1..def8159590b 100644 --- a/src/hotspot/share/opto/classes.hpp +++ b/src/hotspot/share/opto/classes.hpp @@ -273,6 +273,7 @@ macro(NegL) macro(NegD) macro(NegF) macro(NeverBranch) +macro(NarrowMemProj) macro(OnSpinWait) macro(Opaque1) macro(OpaqueLoopInit) diff --git a/src/hotspot/share/opto/escape.cpp b/src/hotspot/share/opto/escape.cpp index 780121ab087..e33e6fa6e86 100644 --- a/src/hotspot/share/opto/escape.cpp +++ b/src/hotspot/share/opto/escape.cpp @@ -863,7 +863,7 @@ Node* ConnectionGraph::split_castpp_load_through_phi(Node* curr_addp, Node* curr // \|/ // Phi # "Field" Phi // -void ConnectionGraph::reduce_phi_on_castpp_field_load(Node* curr_castpp, GrowableArray &alloc_worklist, GrowableArray &memnode_worklist) { +void ConnectionGraph::reduce_phi_on_castpp_field_load(Node* curr_castpp, GrowableArray &alloc_worklist) { Node* ophi = curr_castpp->in(1); assert(ophi->is_Phi(), "Expected this to be a Phi node."); @@ -1279,7 +1279,7 @@ bool ConnectionGraph::reduce_phi_on_safepoints_helper(Node* ophi, Node* cast, No return true; } -void ConnectionGraph::reduce_phi(PhiNode* ophi, GrowableArray &alloc_worklist, GrowableArray &memnode_worklist) { +void ConnectionGraph::reduce_phi(PhiNode* ophi, GrowableArray &alloc_worklist) { bool delay = _igvn->delay_transform(); _igvn->set_delay_transform(true); _igvn->hash_delete(ophi); @@ -1307,7 +1307,7 @@ void ConnectionGraph::reduce_phi(PhiNode* ophi, GrowableArray &alloc_wo // splitting CastPPs we make reference to the inputs of the Cmp that is used // by the If controlling the CastPP. for (uint i = 0; i < castpps.size(); i++) { - reduce_phi_on_castpp_field_load(castpps.at(i), alloc_worklist, memnode_worklist); + reduce_phi_on_castpp_field_load(castpps.at(i), alloc_worklist); } for (uint i = 0; i < others.size(); i++) { @@ -4151,6 +4151,11 @@ Node* ConnectionGraph::find_inst_mem(Node *orig_mem, int alias_idx, GrowableArra // which contains this memory slice, otherwise skip over it. if (alloc == nullptr || alloc->_idx != (uint)toop->instance_id()) { result = proj_in->in(TypeFunc::Memory); + } else if (C->get_alias_index(result->adr_type()) != alias_idx) { + assert(C->get_general_index(alias_idx) == C->get_alias_index(result->adr_type()), "should be projection for the same field/array element"); + result = get_map(result->_idx); + assert(result != nullptr, "new projection should have been allocated"); + break; } } else if (proj_in->is_MemBar()) { // Check if there is an array copy for a clone @@ -4447,6 +4452,22 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, _compile->get_alias_index(tinst->add_offset(oopDesc::mark_offset_in_bytes())); _compile->get_alias_index(tinst->add_offset(oopDesc::klass_offset_in_bytes())); if (alloc->is_Allocate() && (t->isa_instptr() || t->isa_aryptr())) { + // Add a new NarrowMem projection for each existing NarrowMem projection with new adr type + InitializeNode* init = alloc->as_Allocate()->initialization(); + assert(init != nullptr, "can't find Initialization node for this Allocate node"); + auto process_narrow_proj = [&](NarrowMemProjNode* proj) { + const TypePtr* adr_type = proj->adr_type(); + const TypePtr* new_adr_type = tinst->add_offset(adr_type->offset()); + if (adr_type != new_adr_type && !init->already_has_narrow_mem_proj_with_adr_type(new_adr_type)) { + DEBUG_ONLY( uint alias_idx = _compile->get_alias_index(new_adr_type); ) + assert(_compile->get_general_index(alias_idx) == _compile->get_alias_index(adr_type), "new adr type should be narrowed down from existing adr type"); + NarrowMemProjNode* new_proj = new NarrowMemProjNode(init, new_adr_type); + igvn->set_type(new_proj, new_proj->bottom_type()); + record_for_optimizer(new_proj); + set_map(proj, new_proj); // record it so ConnectionGraph::find_inst_mem() can find it + } + }; + init->for_each_narrow_mem_proj_with_new_uses(process_narrow_proj); // First, put on the worklist all Field edges from Connection Graph // which is more accurate than putting immediate users from Ideal Graph. @@ -4518,7 +4539,7 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, // finishes. For now we just try to split out the SR inputs of the merge. Node* parent = n->in(1); if (reducible_merges.member(n)) { - reduce_phi(n->as_Phi(), alloc_worklist, memnode_worklist); + reduce_phi(n->as_Phi(), alloc_worklist); #ifdef ASSERT if (VerifyReduceAllocationMerges) { reduced_merges.push(n); @@ -4710,11 +4731,13 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, } if (n->is_Phi() || n->is_ClearArray()) { // we don't need to do anything, but the users must be pushed - } else if (n->is_MemBar()) { // Initialize, MemBar nodes - // we don't need to do anything, but the users must be pushed - n = n->as_MemBar()->proj_out_or_null(TypeFunc::Memory); - if (n == nullptr) { - continue; + } else if (n->is_MemBar()) { // MemBar nodes + if (!n->is_Initialize()) { // memory projections for Initialize pushed below (so we get to all their uses) + // we don't need to do anything, but the users must be pushed + n = n->as_MemBar()->proj_out_or_null(TypeFunc::Memory); + if (n == nullptr) { + continue; + } } } else if (n->is_CallLeaf()) { // Runtime calls with narrow memory input (no MergeMem node) @@ -4731,6 +4754,8 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, // get the memory projection n = n->find_out_with(Op_SCMemProj); assert(n != nullptr && n->Opcode() == Op_SCMemProj, "memory projection required"); + } else if (n->is_Proj()) { + assert(n->in(0)->is_Initialize(), "we only push memory projections for Initialize"); } else { #ifdef ASSERT if (!n->is_Mem()) { @@ -4774,6 +4799,11 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, if (use->in(TypeFunc::Memory) == n) { // Ignore precedent edge memnode_worklist.append_if_missing(use); } + } else if (use->is_Proj()) { + assert(n->is_Initialize(), "We only push projections of Initialize"); + if (use->as_Proj()->_con == TypeFunc::Memory) { // Ignore precedent edge + memnode_worklist.append_if_missing(use); + } #ifdef ASSERT } else if(use->is_Mem()) { assert(use->in(MemNode::Memory) != n, "EA: missing memory path"); @@ -4825,7 +4855,7 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, // First, update mergemem by moving memory nodes to corresponding slices // if their type became more precise since this mergemem was created. while (mem->is_Mem()) { - const Type *at = igvn->type(mem->in(MemNode::Address)); + const Type* at = igvn->type(mem->in(MemNode::Address)); if (at != Type::TOP) { assert (at->isa_ptr() != nullptr, "pointer type required."); uint idx = (uint)_compile->get_alias_index(at->is_ptr()); @@ -4945,7 +4975,7 @@ void ConnectionGraph::split_unique_types(GrowableArray &alloc_worklist, record_for_optimizer(n); } else { assert(n->is_Allocate() || n->is_CheckCastPP() || - n->is_AddP() || n->is_Phi(), "unknown node used for set_map()"); + n->is_AddP() || n->is_Phi() || n->is_NarrowMemProj(), "unknown node used for set_map()"); } } #if 0 // ifdef ASSERT diff --git a/src/hotspot/share/opto/escape.hpp b/src/hotspot/share/opto/escape.hpp index 0b8cd3aa138..77d14525383 100644 --- a/src/hotspot/share/opto/escape.hpp +++ b/src/hotspot/share/opto/escape.hpp @@ -563,8 +563,10 @@ class ConnectionGraph: public ArenaObj { // Memory Phi - most recent unique Phi split out // from this Phi // MemNode - new memory input for this node - // ChecCastPP - allocation that this is a cast of + // CheckCastPP - allocation that this is a cast of // allocation - CheckCastPP of the allocation + // NarrowMem - newly created projection (type includes instance_id) from projection created + // before EA // manage entries in _node_map @@ -609,11 +611,11 @@ class ConnectionGraph: public ArenaObj { bool can_reduce_phi_check_inputs(PhiNode* ophi) const; void reduce_phi_on_field_access(Node* previous_addp, GrowableArray &alloc_worklist); - void reduce_phi_on_castpp_field_load(Node* castpp, GrowableArray &alloc_worklist, GrowableArray &memnode_worklist); + void reduce_phi_on_castpp_field_load(Node* castpp, GrowableArray &alloc_worklist); void reduce_phi_on_cmp(Node* cmp); bool reduce_phi_on_safepoints(PhiNode* ophi); bool reduce_phi_on_safepoints_helper(Node* ophi, Node* cast, Node* selector, Unique_Node_List& safepoints); - void reduce_phi(PhiNode* ophi, GrowableArray &alloc_worklist, GrowableArray &memnode_worklist); + void reduce_phi(PhiNode* ophi, GrowableArray &alloc_worklist); void set_not_scalar_replaceable(PointsToNode* ptn NOT_PRODUCT(COMMA const char* reason)) const { #ifndef PRODUCT diff --git a/src/hotspot/share/opto/graphKit.cpp b/src/hotspot/share/opto/graphKit.cpp index 20feca26ede..fa9ee63b440 100644 --- a/src/hotspot/share/opto/graphKit.cpp +++ b/src/hotspot/share/opto/graphKit.cpp @@ -3630,14 +3630,17 @@ Node* GraphKit::set_output_for_allocation(AllocateNode* alloc, record_for_igvn(minit_in); // fold it up later, if possible Node* minit_out = memory(rawidx); assert(minit_out->is_Proj() && minit_out->in(0) == init, ""); - // Add an edge in the MergeMem for the header fields so an access - // to one of those has correct memory state - set_memory(minit_out, C->get_alias_index(oop_type->add_offset(oopDesc::mark_offset_in_bytes()))); - set_memory(minit_out, C->get_alias_index(oop_type->add_offset(oopDesc::klass_offset_in_bytes()))); + int mark_idx = C->get_alias_index(oop_type->add_offset(oopDesc::mark_offset_in_bytes())); + // Add an edge in the MergeMem for the header fields so an access to one of those has correct memory state. + // Use one NarrowMemProjNode per slice to properly record the adr type of each slice. The Initialize node will have + // multiple projections as a result. + set_memory(_gvn.transform(new NarrowMemProjNode(init, C->get_adr_type(mark_idx))), mark_idx); + int klass_idx = C->get_alias_index(oop_type->add_offset(oopDesc::klass_offset_in_bytes())); + set_memory(_gvn.transform(new NarrowMemProjNode(init, C->get_adr_type(klass_idx))), klass_idx); if (oop_type->isa_aryptr()) { const TypePtr* telemref = oop_type->add_offset(Type::OffsetBot); int elemidx = C->get_alias_index(telemref); - hook_memory_on_init(*this, elemidx, minit_in, minit_out); + hook_memory_on_init(*this, elemidx, minit_in, _gvn.transform(new NarrowMemProjNode(init, C->get_adr_type(elemidx)))); } else if (oop_type->isa_instptr()) { ciInstanceKlass* ik = oop_type->is_instptr()->instance_klass(); for (int i = 0, len = ik->nof_nonstatic_fields(); i < len; i++) { @@ -3646,7 +3649,7 @@ Node* GraphKit::set_output_for_allocation(AllocateNode* alloc, continue; // do not bother to track really large numbers of fields // Find (or create) the alias category for this field: int fieldidx = C->alias_type(field)->index(); - hook_memory_on_init(*this, fieldidx, minit_in, minit_out); + hook_memory_on_init(*this, fieldidx, minit_in, _gvn.transform(new NarrowMemProjNode(init, C->get_adr_type(fieldidx)))); } } } diff --git a/src/hotspot/share/opto/idealGraphPrinter.cpp b/src/hotspot/share/opto/idealGraphPrinter.cpp index 3721394c112..4883815ebe1 100644 --- a/src/hotspot/share/opto/idealGraphPrinter.cpp +++ b/src/hotspot/share/opto/idealGraphPrinter.cpp @@ -588,7 +588,7 @@ void IdealGraphPrinter::visit_node(Node* n, bool edges) { t->dump_on(&s2); } else if( t == Type::MEMORY ) { s2.print(" Memory:"); - MemNode::dump_adr_type(node, node->adr_type(), &s2); + MemNode::dump_adr_type(node->adr_type(), &s2); } assert(s2.size() < sizeof(buffer), "size in range"); diff --git a/src/hotspot/share/opto/library_call.cpp b/src/hotspot/share/opto/library_call.cpp index f74af38387c..da64972e02d 100644 --- a/src/hotspot/share/opto/library_call.cpp +++ b/src/hotspot/share/opto/library_call.cpp @@ -5518,7 +5518,7 @@ void LibraryCallKit::arraycopy_move_allocation_here(AllocateArrayNode* alloc, No InitializeNode* init = alloc->initialization(); Node* alloc_mem = alloc->in(TypeFunc::Memory); C->gvn_replace_by(callprojs.fallthrough_ioproj, alloc->in(TypeFunc::I_O)); - C->gvn_replace_by(init->proj_out(TypeFunc::Memory), alloc_mem); + init->replace_mem_projs_by(alloc_mem, C); // The CastIINode created in GraphKit::new_array (in AllocateArrayNode::make_ideal_length) must stay below // the allocation (i.e. is only valid if the allocation succeeds): @@ -5569,8 +5569,20 @@ void LibraryCallKit::arraycopy_move_allocation_here(AllocateArrayNode* alloc, No } const TypePtr* telemref = ary_type->add_offset(Type::OffsetBot); int elemidx = C->get_alias_index(telemref); - set_memory(init->proj_out_or_null(TypeFunc::Memory), Compile::AliasIdxRaw); - set_memory(init->proj_out_or_null(TypeFunc::Memory), elemidx); + // Need to properly move every memory projection for the Initialize +#ifdef ASSERT + int mark_idx = C->get_alias_index(ary_type->add_offset(oopDesc::mark_offset_in_bytes())); + int klass_idx = C->get_alias_index(ary_type->add_offset(oopDesc::klass_offset_in_bytes())); +#endif + auto move_proj = [&](ProjNode* proj) { + int alias_idx = C->get_alias_index(proj->adr_type()); + assert(alias_idx == Compile::AliasIdxRaw || + alias_idx == elemidx || + alias_idx == mark_idx || + alias_idx == klass_idx, "should be raw memory or array element type"); + set_memory(proj, alias_idx); + }; + init->for_each_proj(move_proj, TypeFunc::Memory); Node* allocx = _gvn.transform(alloc); assert(allocx == alloc, "where has the allocation gone?"); diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index 58105afd9d5..323680419e6 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -3942,7 +3942,9 @@ bool PhaseIdealLoop::intrinsify_fill(IdealLoopTree* lpt) { call->init_req(TypeFunc::I_O, C->top()); // Does no I/O. call->init_req(TypeFunc::Memory, mem_phi->in(LoopNode::EntryControl)); call->init_req(TypeFunc::ReturnAdr, C->start()->proj_out_or_null(TypeFunc::ReturnAdr)); - call->init_req(TypeFunc::FramePtr, C->start()->proj_out_or_null(TypeFunc::FramePtr)); + Node* frame = new ParmNode(C->start(), TypeFunc::FramePtr); + _igvn.register_new_node_with_optimizer(frame); + call->init_req(TypeFunc::FramePtr, frame); _igvn.register_new_node_with_optimizer(call); result_ctrl = new ProjNode(call,TypeFunc::Control); _igvn.register_new_node_with_optimizer(result_ctrl); diff --git a/src/hotspot/share/opto/loopopts.cpp b/src/hotspot/share/opto/loopopts.cpp index 070749cfe1f..23902d9c5ca 100644 --- a/src/hotspot/share/opto/loopopts.cpp +++ b/src/hotspot/share/opto/loopopts.cpp @@ -2654,8 +2654,9 @@ void PhaseIdealLoop::fix_ctrl_uses(const Node_List& body, const IdealLoopTree* l if (head->is_strip_mined() && mode != IgnoreStripMined) { CountedLoopNode* cl = head->as_CountedLoop(); CountedLoopEndNode* cle = cl->loopexit(); - Node* cle_out = cle->proj_out_or_null(false); - if (use == cle_out) { + // is use the projection that exits the loop from the CountedLoopEndNode? + if (use->in(0) == cle) { + IfFalseNode* cle_out = use->as_IfFalse(); IfNode* le = cl->outer_loop_end(); use = le->proj_out(false); use_loop = get_loop(use); diff --git a/src/hotspot/share/opto/macro.cpp b/src/hotspot/share/opto/macro.cpp index acf1dbabf19..f0845ff12ba 100644 --- a/src/hotspot/share/opto/macro.cpp +++ b/src/hotspot/share/opto/macro.cpp @@ -1048,7 +1048,6 @@ void PhaseMacroExpand::process_users_of_allocation(CallNode *alloc) { if (use->is_Initialize()) { // Eliminate Initialize node. InitializeNode *init = use->as_Initialize(); - assert(init->outcnt() <= 2, "only a control and memory projection expected"); Node *ctrl_proj = init->proj_out_or_null(TypeFunc::Control); if (ctrl_proj != nullptr) { _igvn.replace_node(ctrl_proj, init->in(TypeFunc::Control)); @@ -1058,18 +1057,18 @@ void PhaseMacroExpand::process_users_of_allocation(CallNode *alloc) { assert(tmp == nullptr || tmp == _callprojs.fallthrough_catchproj, "allocation control projection"); #endif } - Node *mem_proj = init->proj_out_or_null(TypeFunc::Memory); - if (mem_proj != nullptr) { - Node *mem = init->in(TypeFunc::Memory); + Node* mem = init->in(TypeFunc::Memory); #ifdef ASSERT + if (init->number_of_projs(TypeFunc::Memory) > 0) { if (mem->is_MergeMem()) { - assert(mem->in(TypeFunc::Memory) == _callprojs.fallthrough_memproj, "allocation memory projection"); + assert(mem->as_MergeMem()->memory_at(Compile::AliasIdxRaw) == _callprojs.fallthrough_memproj, "allocation memory projection"); } else { assert(mem == _callprojs.fallthrough_memproj, "allocation memory projection"); } -#endif - _igvn.replace_node(mem_proj, mem); } +#endif + init->replace_mem_projs_by(mem, &_igvn); + assert(init->outcnt() == 0, "should only have had a control and some memory projections, and we removed them"); } else { assert(false, "only Initialize or AddP expected"); } @@ -1656,7 +1655,16 @@ void PhaseMacroExpand::expand_initialize_membar(AllocateNode* alloc, InitializeN // No InitializeNode or no stores captured by zeroing // elimination. Simply add the MemBarStoreStore after object // initialization. - MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxBot); + // What we want is to prevent the compiler and the CPU from re-ordering the stores that initialize this object + // with subsequent stores to any slice. As a consequence, this MemBar should capture the entire memory state at + // this point in the IR and produce a new memory state that should cover all slices. However, the Initialize node + // only captures/produces a partial memory state making it complicated to insert such a MemBar. Because + // re-ordering by the compiler can't happen by construction (a later Store that publishes the just allocated + // object reference is indirectly control dependent on the Initialize node), preventing reordering by the CPU is + // sufficient. For that a MemBar on the raw memory slice is good enough. + // If init is null, this allocation does have an InitializeNode but this logic can't locate it (see comment in + // PhaseMacroExpand::initialize_object()). + MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxRaw); transform_later(mb); mb->init_req(TypeFunc::Memory, fast_oop_rawmem); @@ -1672,24 +1680,33 @@ void PhaseMacroExpand::expand_initialize_membar(AllocateNode* alloc, InitializeN // barrier. Node* init_ctrl = init->proj_out_or_null(TypeFunc::Control); - Node* init_mem = init->proj_out_or_null(TypeFunc::Memory); - MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxBot); + // See comment above that explains why a raw memory MemBar is good enough. + MemBarNode* mb = MemBarNode::make(C, Op_MemBarStoreStore, Compile::AliasIdxRaw); transform_later(mb); Node* ctrl = new ProjNode(init, TypeFunc::Control); transform_later(ctrl); - Node* mem = new ProjNode(init, TypeFunc::Memory); - transform_later(mem); + Node* old_raw_mem_proj = nullptr; + auto find_raw_mem = [&](ProjNode* proj) { + if (C->get_alias_index(proj->adr_type()) == Compile::AliasIdxRaw) { + assert(old_raw_mem_proj == nullptr, "only one expected"); + old_raw_mem_proj = proj; + } + }; + init->for_each_proj(find_raw_mem, TypeFunc::Memory); + assert(old_raw_mem_proj != nullptr, "should have found raw mem Proj"); + Node* raw_mem_proj = new ProjNode(init, TypeFunc::Memory); + transform_later(raw_mem_proj); // The MemBarStoreStore depends on control and memory coming // from the InitializeNode - mb->init_req(TypeFunc::Memory, mem); + mb->init_req(TypeFunc::Memory, raw_mem_proj); mb->init_req(TypeFunc::Control, ctrl); ctrl = new ProjNode(mb, TypeFunc::Control); transform_later(ctrl); - mem = new ProjNode(mb, TypeFunc::Memory); + Node* mem = new ProjNode(mb, TypeFunc::Memory); transform_later(mem); // All nodes that depended on the InitializeNode for control @@ -1698,9 +1715,7 @@ void PhaseMacroExpand::expand_initialize_membar(AllocateNode* alloc, InitializeN if (init_ctrl != nullptr) { _igvn.replace_node(init_ctrl, ctrl); } - if (init_mem != nullptr) { - _igvn.replace_node(init_mem, mem); - } + _igvn.replace_node(old_raw_mem_proj, mem); } } } diff --git a/src/hotspot/share/opto/matcher.cpp b/src/hotspot/share/opto/matcher.cpp index 0849b40ad7e..beeca961110 100644 --- a/src/hotspot/share/opto/matcher.cpp +++ b/src/hotspot/share/opto/matcher.cpp @@ -173,6 +173,8 @@ void Matcher::verify_new_nodes_only(Node* xroot) { continue; } assert(C->node_arena()->contains(n), "dead node"); + assert(!n->is_Initialize() || n->as_Initialize()->number_of_projs(TypeFunc::Memory) == 1, + "after matching, Initialize should have a single memory projection"); for (uint j = 0; j < n->req(); j++) { Node* in = n->in(j); if (in != nullptr) { @@ -1149,7 +1151,7 @@ Node *Matcher::xform( Node *n, int max_stack ) { // Old-space or new-space check if (!C->node_arena()->contains(n)) { // Old space! - Node* m; + Node* m = nullptr; if (has_new_node(n)) { // Not yet Label/Reduced m = new_node(n); } else { @@ -1164,9 +1166,21 @@ Node *Matcher::xform( Node *n, int max_stack ) { } } else { // Nothing the matcher cares about if (n->is_Proj() && n->in(0) != nullptr && n->in(0)->is_Multi()) { // Projections? - // Convert to machine-dependent projection - m = n->in(0)->as_Multi()->match( n->as_Proj(), this ); - NOT_PRODUCT(record_new2old(m, n);) + if (n->in(0)->is_Initialize() && n->as_Proj()->_con == TypeFunc::Memory) { + // Initialize may have multiple NarrowMem projections. They would all match to identical raw mem MachProjs. + // We don't need multiple MachProjs. Create one if none already exist, otherwise use existing one. + m = n->in(0)->as_Initialize()->mem_mach_proj(); + if (m == nullptr && has_new_node(n->in(0))) { + InitializeNode* new_init = new_node(n->in(0))->as_Initialize(); + m = new_init->mem_mach_proj(); + } + assert(m == nullptr || m->is_MachProj(), "no mem projection yet or a MachProj created during matching"); + } + if (m == nullptr) { + // Convert to machine-dependent projection + m = n->in(0)->as_Multi()->match( n->as_Proj(), this ); + NOT_PRODUCT(record_new2old(m, n);) + } if (m->in(0) != nullptr) // m might be top collect_null_checks(m, n); } else { // Else just a regular 'ol guy diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp index 27565ea397e..8f38da7de75 100644 --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -91,7 +91,7 @@ void MemNode::dump_spec(outputStream *st) const { if (in(Address) != nullptr) _adr_type = in(Address)->bottom_type()->isa_ptr(); #endif - dump_adr_type(this, _adr_type, st); + dump_adr_type(_adr_type, st); Compile* C = Compile::current(); if (C->alias_type(_adr_type)->is_volatile()) { @@ -108,7 +108,7 @@ void MemNode::dump_spec(outputStream *st) const { } } -void MemNode::dump_adr_type(const Node* mem, const TypePtr* adr_type, outputStream *st) { +void MemNode::dump_adr_type(const TypePtr* adr_type, outputStream* st) { st->print(" @"); if (adr_type == nullptr) { st->print("null"); @@ -4231,7 +4231,7 @@ MemBarNode* MemBarNode::make(Compile* C, int opcode, int atp, Node* pn) { } void MemBarNode::remove(PhaseIterGVN *igvn) { - assert(outcnt() > 0 && outcnt() <= 2, "Only one or two out edges allowed"); + assert(outcnt() > 0 && (outcnt() <= 2 || Opcode() == Op_Initialize), "Only one or two out edges allowed"); if (trailing_store() || trailing_load_store()) { MemBarNode* leading = leading_membar(); if (leading != nullptr) { @@ -4239,12 +4239,16 @@ void MemBarNode::remove(PhaseIterGVN *igvn) { leading->remove(igvn); } } - if (proj_out_or_null(TypeFunc::Memory) != nullptr) { - igvn->replace_node(proj_out(TypeFunc::Memory), in(TypeFunc::Memory)); - } if (proj_out_or_null(TypeFunc::Control) != nullptr) { igvn->replace_node(proj_out(TypeFunc::Control), in(TypeFunc::Control)); } + if (is_Initialize()) { + as_Initialize()->replace_mem_projs_by(in(TypeFunc::Memory), igvn); + } else { + if (proj_out_or_null(TypeFunc::Memory) != nullptr) { + igvn->replace_node(proj_out(TypeFunc::Memory), in(TypeFunc::Memory)); + } + } } //------------------------------Ideal------------------------------------------ @@ -5447,6 +5451,48 @@ Node* InitializeNode::complete_stores(Node* rawctl, Node* rawmem, Node* rawptr, return rawmem; } +void InitializeNode::replace_mem_projs_by(Node* mem, Compile* C) { + auto replace_proj = [&](ProjNode* proj) { + C->gvn_replace_by(proj, mem); + return CONTINUE; + }; + apply_to_projs(replace_proj, TypeFunc::Memory); +} + +void InitializeNode::replace_mem_projs_by(Node* mem, PhaseIterGVN* igvn) { + DUIterator_Fast imax, i = fast_outs(imax); + auto replace_proj = [&](ProjNode* proj) { + igvn->replace_node(proj, mem); + --i; --imax; + return CONTINUE; + }; + apply_to_projs(imax, i, replace_proj, TypeFunc::Memory); +} + +bool InitializeNode::already_has_narrow_mem_proj_with_adr_type(const TypePtr* adr_type) const { + auto find_proj = [&](ProjNode* proj) { + if (proj->adr_type() == adr_type) { + return BREAK_AND_RETURN_CURRENT_PROJ; + } + return CONTINUE; + }; + DUIterator_Fast imax, i = fast_outs(imax); + return apply_to_narrow_mem_projs_any_iterator(UsesIteratorFast(imax, i, this), find_proj) != nullptr; +} + +MachProjNode* InitializeNode::mem_mach_proj() const { + auto find_proj = [](ProjNode* proj) { + if (proj->is_MachProj()) { + return BREAK_AND_RETURN_CURRENT_PROJ; + } + return CONTINUE; + }; + ProjNode* proj = apply_to_projs(find_proj, TypeFunc::Memory); + if (proj == nullptr) { + return nullptr; + } + return proj->as_MachProj(); +} #ifdef ASSERT bool InitializeNode::stores_are_sane(PhaseValues* phase) { @@ -5869,6 +5915,7 @@ Node* MergeMemNode::memory_at(uint alias_idx) const { || n->adr_type() == nullptr // address is TOP || n->adr_type() == TypePtr::BOTTOM || n->adr_type() == TypeRawPtr::BOTTOM + || n->is_NarrowMemProj() || !Compile::current()->do_aliasing(), "must be a wide memory"); // do_aliasing == false if we are organizing the memory states manually. diff --git a/src/hotspot/share/opto/memnode.hpp b/src/hotspot/share/opto/memnode.hpp index 810a9fe9445..d554c037012 100644 --- a/src/hotspot/share/opto/memnode.hpp +++ b/src/hotspot/share/opto/memnode.hpp @@ -167,7 +167,7 @@ class MemNode : public Node { bool is_unsafe_access() const { return _unsafe_access; } #ifndef PRODUCT - static void dump_adr_type(const Node* mem, const TypePtr* adr_type, outputStream *st); + static void dump_adr_type(const TypePtr* adr_type, outputStream* st); virtual void dump_spec(outputStream *st) const; #endif }; @@ -1371,7 +1371,20 @@ class InitializeNode: public MemBarNode { intptr_t header_size, Node* size_in_bytes, PhaseIterGVN* phase); - private: + // An Initialize node has multiple memory projections. Helper methods used when the node is removed. + // For use at parse time + void replace_mem_projs_by(Node* mem, Compile* C); + // For use with IGVN + void replace_mem_projs_by(Node* mem, PhaseIterGVN* igvn); + + // Does a NarrowMemProj with this adr_type and this node as input already exist? + bool already_has_narrow_mem_proj_with_adr_type(const TypePtr* adr_type) const; + + // Used during matching: find the MachProj memory projection if there's one. Expectation is that there should be at + // most one. + MachProjNode* mem_mach_proj() const; + +private: void remove_extra_zeroes(); // Find out where a captured store should be placed (or already is placed). @@ -1388,6 +1401,33 @@ class InitializeNode: public MemBarNode { PhaseGVN* phase); intptr_t find_next_fullword_store(uint i, PhaseGVN* phase); + + // Iterate with i over all NarrowMemProj uses calling callback + template NarrowMemProjNode* apply_to_narrow_mem_projs_any_iterator(Iterator i, Callback callback) const { + auto filter = [&](ProjNode* proj) { + if (proj->is_NarrowMemProj() && callback(proj->as_NarrowMemProj()) == BREAK_AND_RETURN_CURRENT_PROJ) { + return BREAK_AND_RETURN_CURRENT_PROJ; + } + return CONTINUE; + }; + ProjNode* res = apply_to_projs_any_iterator(i, filter); + if (res == nullptr) { + return nullptr; + } + return res->as_NarrowMemProj(); + } + +public: + + // callback is allowed to add new uses that will then be iterated over + template void for_each_narrow_mem_proj_with_new_uses(Callback callback) const { + auto callback_always_continue = [&](NarrowMemProjNode* proj) { + callback(proj); + return MultiNode::CONTINUE; + }; + DUIterator i = outs(); + apply_to_narrow_mem_projs_any_iterator(UsesIterator(i, this), callback_always_continue); + } }; //------------------------------MergeMem--------------------------------------- diff --git a/src/hotspot/share/opto/multnode.cpp b/src/hotspot/share/opto/multnode.cpp index 736e84315ee..d7058c198dd 100644 --- a/src/hotspot/share/opto/multnode.cpp +++ b/src/hotspot/share/opto/multnode.cpp @@ -45,30 +45,58 @@ Node *MultiNode::match( const ProjNode *proj, const Matcher *m ) { return proj-> // Get a named projection or null if not found ProjNode* MultiNode::proj_out_or_null(uint which_proj) const { assert((Opcode() != Op_If && Opcode() != Op_RangeCheck) || which_proj == (uint)true || which_proj == (uint)false, "must be 1 or 0"); - for( DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++ ) { - Node *p = fast_out(i); - if (p->is_Proj()) { - ProjNode *proj = p->as_Proj(); - if (proj->_con == which_proj) { - assert((Opcode() != Op_If && Opcode() != Op_RangeCheck) || proj->Opcode() == (which_proj ? Op_IfTrue : Op_IfFalse), "bad if #2"); - return proj; - } - } else { - assert(p == this && this->is_Start(), "else must be proj"); - continue; - } - } - return nullptr; + assert(number_of_projs(which_proj) <= 1, "only when there's a single projection"); + ProjNode* proj = find_first(which_proj); + assert(proj == nullptr || (Opcode() != Op_If && Opcode() != Op_RangeCheck) || proj->Opcode() == (which_proj ? Op_IfTrue : Op_IfFalse), + "incorrect projection node at If/RangeCheck: IfTrue on false path or IfFalse on true path"); + return proj; } ProjNode* MultiNode::proj_out_or_null(uint which_proj, bool is_io_use) const { - for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) { - ProjNode* proj = fast_out(i)->isa_Proj(); - if (proj != nullptr && (proj->_con == which_proj) && (proj->_is_io_use == is_io_use)) { - return proj; + assert(number_of_projs(which_proj, is_io_use) <= 1, "only when there's a single projection"); + return find_first(which_proj, is_io_use); +} + +template ProjNode* MultiNode::apply_to_projs(Callback callback, uint which_proj, bool is_io_use) const { + auto filter = [&](ProjNode* proj) { + if (proj->_is_io_use == is_io_use && callback(proj) == BREAK_AND_RETURN_CURRENT_PROJ) { + return BREAK_AND_RETURN_CURRENT_PROJ; } - } - return nullptr; + return CONTINUE; + }; + return apply_to_projs(filter, which_proj); +} + +uint MultiNode::number_of_projs(uint which_proj) const { + uint cnt = 0; + auto count_projs = [&](ProjNode* proj) { + cnt++; + }; + for_each_proj(count_projs, which_proj); + return cnt; +} + +uint MultiNode::number_of_projs(uint which_proj, bool is_io_use) const { + uint cnt = 0; + auto count_projs = [&](ProjNode* proj) { + cnt++; + }; + for_each_proj(count_projs, which_proj, is_io_use); + return cnt; +} + +ProjNode* MultiNode::find_first(uint which_proj) const { + auto find_proj = [&](ProjNode* proj) { + return BREAK_AND_RETURN_CURRENT_PROJ; + }; + return apply_to_projs(find_proj, which_proj); +} + +ProjNode* MultiNode::find_first(uint which_proj, bool is_io_use) const { + auto find_proj = [](ProjNode* proj) { + return BREAK_AND_RETURN_CURRENT_PROJ; + }; + return apply_to_projs(find_proj, which_proj, is_io_use); } // Get a named projection @@ -226,3 +254,8 @@ ProjNode* ProjNode::other_if_proj() const { assert(_con == 0 || _con == 1, "not an if?"); return in(0)->as_If()->proj_out(1-_con); } + +NarrowMemProjNode::NarrowMemProjNode(InitializeNode* src, const TypePtr* adr_type) + : ProjNode(src, TypeFunc::Memory), _adr_type(adr_type) { + init_class_id(Class_NarrowMemProj); +} diff --git a/src/hotspot/share/opto/multnode.hpp b/src/hotspot/share/opto/multnode.hpp index dff2caed38d..3d6d732145c 100644 --- a/src/hotspot/share/opto/multnode.hpp +++ b/src/hotspot/share/opto/multnode.hpp @@ -49,6 +49,105 @@ class MultiNode : public Node { ProjNode* proj_out(uint which_proj) const; // Get a named projection ProjNode* proj_out_or_null(uint which_proj) const; ProjNode* proj_out_or_null(uint which_proj, bool is_io_use) const; + uint number_of_projs(uint which_proj) const; + uint number_of_projs(uint which_proj, bool is_io_use) const; + +protected: + + // Provide single interface for DUIterator_Fast/DUIterator for template method below + class UsesIteratorFast { + DUIterator_Fast& _imax; + DUIterator_Fast& _i; + const Node* _node; + + public: + bool cont() { + return _i < _imax; + } + void next() { + _i++; + } + Node* current() { + return _node->fast_out(_i); + } + UsesIteratorFast(DUIterator_Fast& imax, DUIterator_Fast& i, const Node* node) + : _imax(imax), _i(i), _node(node) { + } + }; + + class UsesIterator { + DUIterator& _i; + const Node* _node; + + public: + bool cont() { + return _node->has_out(_i); + } + void next() { + _i++; + } + Node* current() { + return _node->out(_i); + } + UsesIterator(DUIterator& i, const Node* node) + : _i(i), _node(node) { + } + }; + + // Iterate with i over all Proj uses calling callback + template ProjNode* apply_to_projs_any_iterator(Iterator i, Callback callback) const { + for (; i.cont(); i.next()) { + Node* p = i.current(); + if (p->is_Proj()) { + ProjNode* proj = p->as_Proj(); + ApplyToProjs result = callback(proj); + if (result == BREAK_AND_RETURN_CURRENT_PROJ) { + return proj; + } + assert(result == CONTINUE, "should be either break or continue"); + } else { + assert(p == this && is_Start(), "else must be proj"); + } + } + return nullptr; + } + enum ApplyToProjs { + CONTINUE, + BREAK_AND_RETURN_CURRENT_PROJ + }; + + // Run callback on projections with iterator passed as argument + template ProjNode* apply_to_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback, uint which_proj) const; + + // Same but with default iterator and for matching _con + template ProjNode* apply_to_projs(Callback callback, uint which_proj) const { + DUIterator_Fast imax, i = fast_outs(imax); + return apply_to_projs(imax, i, callback, which_proj); + } + + // Same but for matching _con and _is_io_use + template ProjNode* apply_to_projs(Callback callback, uint which_proj, bool is_io_use) const; + +public: + template void for_each_proj(Callback callback, uint which_proj) const { + auto callback_always_continue = [&](ProjNode* proj) { + callback(proj); + return MultiNode::CONTINUE; + }; + apply_to_projs(callback_always_continue, which_proj); + } + + template void for_each_proj(Callback callback, uint which_proj, bool is_io_use) const { + auto callback_always_continue = [&](ProjNode* proj) { + callback(proj); + return MultiNode::CONTINUE; + }; + apply_to_projs(callback_always_continue, which_proj, is_io_use); + } + + + ProjNode* find_first(uint which_proj) const; + ProjNode* find_first(uint which_proj, bool is_io_use) const; }; //------------------------------ProjNode--------------------------------------- @@ -105,4 +204,39 @@ class ProjNode : public Node { ProjNode* other_if_proj() const; }; +// A ProjNode variant that captures an adr_type(). Used as a projection of InitializeNode to have the right adr_type() +// for array elements/fields. +class NarrowMemProjNode : public ProjNode { +private: + const TypePtr* const _adr_type; +protected: + virtual uint hash() const { + return ProjNode::hash() + _adr_type->hash(); + } + virtual bool cmp(const Node& n) const { + return ProjNode::cmp(n) && ((NarrowMemProjNode&)n)._adr_type == _adr_type; + } + virtual uint size_of() const { + return sizeof(*this); + } +public: + NarrowMemProjNode(InitializeNode* src, const TypePtr* adr_type); + + virtual const TypePtr* adr_type() const { + return _adr_type; + } + + virtual int Opcode() const; +}; + +template ProjNode* MultiNode::apply_to_projs(DUIterator_Fast& imax, DUIterator_Fast& i, Callback callback, uint which_proj) const { + auto filter = [&](ProjNode* proj) { + if (proj->_con == which_proj && callback(proj) == BREAK_AND_RETURN_CURRENT_PROJ) { + return BREAK_AND_RETURN_CURRENT_PROJ; + } + return CONTINUE; + }; + return apply_to_projs_any_iterator(UsesIteratorFast(imax, i, this), filter); +} + #endif // SHARE_OPTO_MULTNODE_HPP diff --git a/src/hotspot/share/opto/node.cpp b/src/hotspot/share/opto/node.cpp index 8f6c67c16f5..75d6757800c 100644 --- a/src/hotspot/share/opto/node.cpp +++ b/src/hotspot/share/opto/node.cpp @@ -2591,7 +2591,7 @@ void Node::dump(const char* suffix, bool mark, outputStream* st, DumpConfig* dc) t->dump_on(st); } else if (t == Type::MEMORY) { st->print(" Memory:"); - MemNode::dump_adr_type(this, adr_type(), st); + MemNode::dump_adr_type(adr_type(), st); } else if (Verbose || WizardMode) { st->print(" Type:"); if (t) { diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 2bbb10879f5..a9c9e71a9d1 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -133,6 +133,7 @@ class MoveNode; class MulNode; class MultiNode; class MultiBranchNode; +class NarrowMemProjNode; class NegNode; class NegVNode; class NeverBranchNode; @@ -762,6 +763,7 @@ class Node { DEFINE_CLASS_ID(IfFalse, IfProj, 1) DEFINE_CLASS_ID(Parm, Proj, 4) DEFINE_CLASS_ID(MachProj, Proj, 5) + DEFINE_CLASS_ID(NarrowMemProj, Proj, 6) DEFINE_CLASS_ID(Mem, Node, 4) DEFINE_CLASS_ID(Load, Mem, 0) @@ -979,6 +981,7 @@ class Node { DEFINE_CLASS_QUERY(Multi) DEFINE_CLASS_QUERY(MultiBranch) DEFINE_CLASS_QUERY(MulVL) + DEFINE_CLASS_QUERY(NarrowMemProj) DEFINE_CLASS_QUERY(Neg) DEFINE_CLASS_QUERY(NegV) DEFINE_CLASS_QUERY(NeverBranch) diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 08a9fcc5490..d405db1571b 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1667,12 +1667,14 @@ void PhaseIterGVN::add_users_of_use_to_worklist(Node* n, Node* use, Unique_Node_ } } } + auto enqueue_init_mem_projs = [&](ProjNode* proj) { + add_users_to_worklist0(proj, worklist); + }; // If changed initialization activity, check dependent Stores if (use_op == Op_Allocate || use_op == Op_AllocateArray) { InitializeNode* init = use->as_Allocate()->initialization(); if (init != nullptr) { - Node* imem = init->proj_out_or_null(TypeFunc::Memory); - if (imem != nullptr) add_users_to_worklist0(imem, worklist); + init->for_each_proj(enqueue_init_mem_projs, TypeFunc::Memory); } } // If the ValidLengthTest input changes then the fallthrough path out of the AllocateArray may have become dead. @@ -1686,8 +1688,8 @@ void PhaseIterGVN::add_users_of_use_to_worklist(Node* n, Node* use, Unique_Node_ } if (use_op == Op_Initialize) { - Node* imem = use->as_Initialize()->proj_out_or_null(TypeFunc::Memory); - if (imem != nullptr) add_users_to_worklist0(imem, worklist); + InitializeNode* init = use->as_Initialize(); + init->for_each_proj(enqueue_init_mem_projs, TypeFunc::Memory); } // Loading the java mirror from a Klass requires two loads and the type // of the mirror load depends on the type of 'n'. See LoadNode::Value(). diff --git a/src/hotspot/share/opto/stringopts.cpp b/src/hotspot/share/opto/stringopts.cpp index 28936a04219..e410463a60b 100644 --- a/src/hotspot/share/opto/stringopts.cpp +++ b/src/hotspot/share/opto/stringopts.cpp @@ -360,17 +360,13 @@ void StringConcat::eliminate_initialize(InitializeNode* init) { Compile* C = _stringopts->C; // Eliminate Initialize node. - assert(init->outcnt() <= 2, "only a control and memory projection expected"); assert(init->req() <= InitializeNode::RawStores, "no pending inits"); Node *ctrl_proj = init->proj_out_or_null(TypeFunc::Control); if (ctrl_proj != nullptr) { C->gvn_replace_by(ctrl_proj, init->in(TypeFunc::Control)); } - Node *mem_proj = init->proj_out_or_null(TypeFunc::Memory); - if (mem_proj != nullptr) { - Node *mem = init->in(TypeFunc::Memory); - C->gvn_replace_by(mem_proj, mem); - } + Node* mem = init->in(TypeFunc::Memory); + init->replace_mem_projs_by(mem, C); C->gvn_replace_by(init, C->top()); init->disconnect_inputs(C); } diff --git a/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/java/com/sun/hotspot/igv/servercompiler/ServerCompilerScheduler.java b/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/java/com/sun/hotspot/igv/servercompiler/ServerCompilerScheduler.java index 0a069c53a71..b53d8b1a0d1 100644 --- a/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/java/com/sun/hotspot/igv/servercompiler/ServerCompilerScheduler.java +++ b/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/java/com/sun/hotspot/igv/servercompiler/ServerCompilerScheduler.java @@ -653,7 +653,9 @@ private static boolean isPhi(Node n) { } private static boolean isProj(Node n) { - return hasName(n, "Proj") || hasName(n, "MachProj"); + return hasName(n, "Proj") || + hasName(n, "MachProj") || + hasName(n, "NarrowMemProj"); } private static boolean isParm(Node n) { diff --git a/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/resources/com/sun/hotspot/igv/servercompiler/filters/condenseGraph.filter b/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/resources/com/sun/hotspot/igv/servercompiler/filters/condenseGraph.filter index 6919f06da00..5e9de455e1f 100644 --- a/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/resources/com/sun/hotspot/igv/servercompiler/filters/condenseGraph.filter +++ b/src/utils/IdealGraphVisualizer/ServerCompiler/src/main/resources/com/sun/hotspot/igv/servercompiler/filters/condenseGraph.filter @@ -53,6 +53,7 @@ split(and([matches("name", "Parm|MachProj"), // Combine single-input nodes. combine(anyNode, matches("name", "Proj|IfFalse|IfTrue|JProj|MachProj|JumpProj|CatchProj|Parm")); +combine(anyNode, matches("name", "NarrowMemProj"), ["N"]); combine(anyNode, matches("name", "SCMemProj"), ["SCM"]); combine(matches("name", "SubTypeCheck|Cmp.*"), matches("name", "Bool"), ["[dump_spec]"]); combine(anyNode, matches("name", "Decode(N|NarrowPtr|NKlass)"), ["DC"]); diff --git a/test/hotspot/jtreg/compiler/escapeAnalysis/TestIterativeEA.java b/test/hotspot/jtreg/compiler/escapeAnalysis/TestIterativeEA.java index 299f98995a0..a5688ef8041 100644 --- a/test/hotspot/jtreg/compiler/escapeAnalysis/TestIterativeEA.java +++ b/test/hotspot/jtreg/compiler/escapeAnalysis/TestIterativeEA.java @@ -48,9 +48,13 @@ public static void main(String[] args) throws Exception { System.out.println(analyzer.getOutput()); analyzer.shouldHaveExitValue(0); - analyzer.shouldContain("++++ Eliminated: 26 Allocate"); - analyzer.shouldContain("++++ Eliminated: 48 Allocate"); - analyzer.shouldContain("++++ Eliminated: 78 Allocate"); + analyzer.shouldMatch( + "(?s)" + // Let .* also match line terminators. + "Eliminated: \\d+ Allocate" + + ".*" + + "Eliminated: \\d+ Allocate" + + ".*" + + "Eliminated: \\d+ Allocate"); } static class A { diff --git a/test/hotspot/jtreg/compiler/macronodes/TestEarlyEliminationOfAllocationWithoutUse.java b/test/hotspot/jtreg/compiler/macronodes/TestEarlyEliminationOfAllocationWithoutUse.java new file mode 100644 index 00000000000..5987bb0bb4b --- /dev/null +++ b/test/hotspot/jtreg/compiler/macronodes/TestEarlyEliminationOfAllocationWithoutUse.java @@ -0,0 +1,67 @@ +/* + * 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 8327963 + * @summary C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed + * @run main/othervm -XX:-BackgroundCompilation compiler.macronodes.TestEarlyEliminationOfAllocationWithoutUse + * @run main/othervm compiler.macronodes.TestEarlyEliminationOfAllocationWithoutUse + */ + +package compiler.macronodes; +import java.util.Arrays; + +public class TestEarlyEliminationOfAllocationWithoutUse { + private static volatile int volatileField; + + public static void main(String[] args) { + boolean[] allTrue = new boolean[3]; + Arrays.fill(allTrue, true); + A a = new A(); + boolean[] allFalse = new boolean[3]; + for (int i = 0; i < 20_000; i++) { + a.field1 = 0; + test1(a, allTrue); + test1(a, allFalse); + if (a.field1 != 42) { + throw new RuntimeException("Lost Store"); + } + } + } + + private static void test1(A otherA, boolean[] flags) { + otherA.field1 = 42; + // Fully unrolled before EA + for (int i = 0; i < 3; i++) { + A a = new A(); // removed right after EA + if (flags[i]) { + break; + } + } + } + + private static class A { + int field1; + } +} diff --git a/test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java b/test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java new file mode 100644 index 00000000000..9eee4c555bf --- /dev/null +++ b/test/hotspot/jtreg/compiler/macronodes/TestEliminationOfAllocationWithoutUse.java @@ -0,0 +1,338 @@ +/* + * Copyright (c) 2024, 2025, Oracle and/or its affiliates. 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 8327012 8327963 + * @summary Revealed issue where hook_memory_on_init links some array slice to the rawptr slice. + * Now that array slice depends on the rawslice. And then when the Initialize MemBar gets + * removed in expand_allocate_common, the rawslice sees that it has now no effect, looks + * through the MergeMem and sees the initial state. That way, also the linked array slice + * goes to the initial state, even if before the allocation there were stores on the array + * slice. This leads to a messed up memory graph, and missing stores in the generated code. + * + * @run main/othervm -Xcomp -XX:-TieredCompilation + * -XX:CompileCommand=compileonly,compiler.macronodes.TestEliminationOfAllocationWithoutUse::test* + * compiler.macronodes.TestEliminationOfAllocationWithoutUse + * @run main/othervm -Xcomp + * -XX:CompileCommand=compileonly,compiler.macronodes.TestEliminationOfAllocationWithoutUse::test* + * compiler.macronodes.TestEliminationOfAllocationWithoutUse + */ + +package compiler.macronodes; + +public class TestEliminationOfAllocationWithoutUse { + + static public void main(String[] args) { + int failures = 0; + failures += run1(); + failures += run2(); + failures += run3(); + failures += run4(); + failures += run5(); + failures += run6(); + failures += run7(); + failures += run8(); + failures += run9(); + if (failures != 0) { + throw new RuntimeException("Had test failures: " + failures); + } + } + + static public int run1() { + int size = 10; + double[] arr1 = new double[size]; + double[] arr2 = new double[size]; + test1(arr1, arr2); + + double sum = 0; + for (int i = 0; i < arr1.length; ++i) { + sum += arr1[i] - arr2[i]; + } + + if (sum != (double)(size)) { + System.out.println("test1: wrong result: " + sum + " vs expected: " + size); + return 1; + } + return 0; + } + + // Simplified from JDK-8327012 regression test. + public static void test1(double[] arr1, double[] arr2) { + for(int i = 0; i < arr1.length; ++i) { + // stores on double[] slice + arr1[i] = (double)(i + 2); + arr2[i] = (double)(i + 1); + // Allocation without use: but Initialize MemBar tangles the rawptr and double[] slices + double[] tmp = new double[100]; + // When the Initialize MemBar is removed, the rawptr slice sees that there is no effect + // and takes the initial state. The double[] slice is hooked on to the rawptr slice, and + // also thinks it has the initial state, ignoring the double[] stores above. + } + } + + static public int run2() { + int size = 10; + double[] arr1 = new double[size]; + test2(arr1); + + double sum = 0; + for(int i = 0; i < arr1.length; ++i) { + sum += arr1[i]; + } + + if (sum != (double)(size)) { + System.out.println("test2: wrong result: " + sum + " vs expected: " + size); + return 1; + } + return 0; + } + + // Simplified from test1 + public static void test2(double[] arr1) { + for(int i = 0; i < arr1.length; ++i) { + arr1[i] = 1; + double[] tmp = new double[100]; + } + } + + static public int run3() { + int size = 10; + int[] arr1 = new int[size]; + test3(arr1); + + int sum = 0; + for(int i = 0; i < arr1.length; ++i) { + sum += arr1[i]; + } + + if (sum != size) { + System.out.println("test3: wrong result: " + sum + " vs expected: " + size); + return 1; + } + return 0; + } + + // Modified from test2 + public static void test3(int[] arr1) { + for(int i = 0; i < arr1.length; ++i) { + arr1[i] = 1; + int[] tmp = new int[100]; + } + } + + // From TestIncorrectResult.java in JDK-8324739 + static int test4(int l2) { + int[] tmp = new int[20]; + + for (int j = 0; j < l2; ++j) { + tmp[j] = 42; + int[] unused_but_necessary = new int[400]; + } + + return tmp[0]; + } + + public static int run4() { + for (int i = 0; i < 100; ++i) { + long res = test4(20); + + if (res != 42) { + System.out.println("test4: wrong result: " + res + " vs expected: 42"); + return 1; + } + } + return 0; + } + + // From JDK-8336701 + static class Test5 { + int[] b = new int[400]; + static int[] staticArray = new int[400]; + } + + static void test5() { + long e; + for (e = 1; e < 9; ++e) { + Test5.staticArray[(int) e] -= e; + synchronized (new Test5()) { } + } + for (int f = 0; f < 10000; ++f) ; + } + + static int run5() { + new Test5(); + for (int i = 0; i < 1000; ++i) { + test5(); + } + if (Test5.staticArray[8] != -8000) { + System.out.println("test5: wrong result: " + Test5.staticArray[8] + " vs expected: -8000"); + return 1; + } + return 0; + } + + // From JDK-8336293 + static class Test6 { + static long c; + static int a = 400; + double[] b = new double[400]; + } + + static void test6() { + long d; + double[] e = new double[Test6.a]; + for (int f = 0; f < e.length; f++) + e[f] = 1.116242; + d = 1; + while (++d < 7) + synchronized (new Test6()) { } + long g = 0; + for (int f = 0; f < e.length; f++) + g += e[f]; + Test6.c += g; + } + + static int run6() { + new Test6(); + for (int f = 0; f < 10000; ++f) { + test6(); + } + if (Test6.c != 4000000) { + System.out.println("test6: wrong result: " + Test6.c + " vs expected: 4000000 "); + return 1; + } + return 0; + } + + // From JDK-8327868 + static class Test7 { + static int a = 400; + int[] b = new int[400]; + static int[] staticArray = new int[a]; + } + + static int test7() { + int l, d = 3; + for (l = 2; 58 > l; l++) { + for (int e = 2; e < 8; e += 2) + for (int f = 1; f < e; f += 2) + synchronized (new Test7()) { + } + do + ; while (d < 2); + int g = 0; + do + g++; + while (g < 20000); + Test7.staticArray[1] -= 3023399; + } + int h = 0; + for (int i = 0; i < Test7.staticArray.length; i++) + h += Test7.staticArray[i]; + return h; + } + + static int run7() { + new Test7(); + int res = test7(); + if (res != -169310344) { + System.out.println("test7: wrong result: " + res + " vs expected: -169310344"); + return 1; + } + return 0; + } + + // from JDK-8329984 + static class Test8 { + static int a = 400; + int[] e = new int[400]; + } + + static int test8() { + int i = 22738; + int b; + int h; + int[] c = new int[Test8.a]; + for (b = 3; b < 273; b++) { + h = 1; + while (++h < 97) switch (b % 6 + 56) { + case 56: + c[1] = i; + case 57: + synchronized (new Test8()) {} + } + } + int k = 0; + for (int j = 0; j < c.length; j++) k += c[j]; + return k; + } + + public static int run8() { + new Test8(); + for (int i = 0; i < 20; i++) { + int res = test8(); + if (res != 22738) { + System.out.println("test8: wrong result: " + res + " vs expected: 22738"); + return 1; + } + } + return 0; + } + + // from JDK-8341009 + static class Test9 { + static int a = 256; + float[] b = new float[256]; + static long c; + } + + static void test9() { + for (int f = 0; f < 10000; ++f) ; + float[][] g = new float[Test9.a][Test9.a]; + for (int d = 7; d < 16; d++) { + long e = 1; + do { + g[d][(int) e] = d; + synchronized (new Test9()) { + } + } while (++e < 5); + } + for (int i = 0; i < Test9.a; ++i) { + for (int j = 0; j < Test9.a ; ++j) { + Test9.c += g[i][j]; + } + } + } + + static int run9() { + for (int j = 6; 116 > j; ++j) { + test9(); + } + if (Test9.c != 43560) { + System.out.println("test9: wrong result: " + Test9.c + " vs expected: 43560"); + return 1; + } + return 0; + } +} diff --git a/test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java b/test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java new file mode 100644 index 00000000000..0176d6a4801 --- /dev/null +++ b/test/hotspot/jtreg/compiler/macronodes/TestInitializingStoreCapturing.java @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2024, 2025, Oracle and/or its affiliates. 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 8327012 8327963 + * @summary Test that initializing store gets captured, i.e. moved before the InitializeNode + * and made into a raw-store. + * @library /test/lib / + * @run driver compiler.macronodes.TestInitializingStoreCapturing + */ + +package compiler.macronodes; + +import compiler.lib.ir_framework.*; + +public class TestInitializingStoreCapturing { + + static class A { + float value; + A(float v) { value = v; } + }; + + static public void main(String[] args) { + TestFramework.run(); + } + + @Test + @IR(counts = {IRNode.STORE_F, "= 0"}) + static A testInitializeField() { + return new A(4.2f); + } + + @Test + @IR(counts = {IRNode.STORE_F, "= 0"}) + static float[] testInitializeArray() { + return new float[] {4.2f}; + } +} From 952018f3c0b3a0aff2f57d145e61748fb0856922 Mon Sep 17 00:00:00 2001 From: Emanuel Peter Date: Thu, 12 Jun 2025 14:19:08 +0000 Subject: [PATCH 3/7] 8347273: C2: VerifyIterativeGVN for Ideal and Identity Reviewed-by: chagedorn, mhaessig --- src/hotspot/share/opto/c2_globals.hpp | 10 +- src/hotspot/share/opto/phaseX.cpp | 914 +++++++++++++++++- src/hotspot/share/opto/phaseX.hpp | 15 +- .../flags/jvmFlagConstraintsCompiler.cpp | 5 +- .../compiler/c2/TestVerifyIterativeGVN.java | 6 +- 5 files changed, 925 insertions(+), 25 deletions(-) diff --git a/src/hotspot/share/opto/c2_globals.hpp b/src/hotspot/share/opto/c2_globals.hpp index fd55f2fd666..227817612ce 100644 --- a/src/hotspot/share/opto/c2_globals.hpp +++ b/src/hotspot/share/opto/c2_globals.hpp @@ -678,10 +678,12 @@ "Print progress during Iterative Global Value Numbering") \ \ develop(uint, VerifyIterativeGVN, 0, \ - "Verify Iterative Global Value Numbering" \ - "=XY, with Y: verify Def-Use modifications during IGVN" \ - " X: verify that type(n) == n->Value() after IGVN" \ - "X and Y in 0=off; 1=on") \ + "Verify Iterative Global Value Numbering =DCBA, with:" \ + " 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" \ + " A: verify Def-Use modifications during IGVN" \ + "Each can be 0=off or 1=on") \ constraint(VerifyIterativeGVNConstraintFunc, AtParse) \ \ develop(bool, TraceCISCSpill, false, \ diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index d405db1571b..c88ac0c3984 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1072,7 +1072,11 @@ void PhaseIterGVN::optimize() { #ifdef ASSERT void PhaseIterGVN::verify_optimize() { - if (is_verify_Value()) { + assert(_worklist.size() == 0, "igvn worklist must be empty before verify"); + + if (is_verify_Value() || + is_verify_Ideal() || + is_verify_Identity()) { ResourceMark rm; Unique_Node_List worklist; bool failure = false; @@ -1080,7 +1084,10 @@ void PhaseIterGVN::verify_optimize() { worklist.push(C->root()); for (uint j = 0; j < worklist.size(); ++j) { Node* n = worklist.at(j); - failure |= verify_node_value(n); + if (is_verify_Value()) { failure |= verify_Value_for(n); } + 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); } // traverse all inputs and outputs for (uint i = 0; i < n->req(); i++) { if (n->in(i) != nullptr) { @@ -1097,6 +1104,27 @@ void PhaseIterGVN::verify_optimize() { // in the verification code above if that is not possible for some reason (like Load nodes). assert(!failure, "Missed optimization opportunity in PhaseIterGVN"); } + + verify_empty_worklist(nullptr); +} + +void PhaseIterGVN::verify_empty_worklist(Node* node) { + // Verify that the igvn worklist is empty. If no optimization happened, then + // nothing needs to be on the worklist. + if (_worklist.size() == 0) { return; } + + stringStream ss; // Print as a block without tty lock. + for (uint j = 0; j < _worklist.size(); j++) { + Node* n = _worklist.at(j); + ss.print("igvn.worklist[%d] ", j); + n->dump("\n", false, &ss); + } + if (_worklist.size() != 0 && node != nullptr) { + ss.print_cr("Previously optimized:"); + node->dump("\n", false, &ss); + } + tty->print_cr("%s", ss.as_string()); + assert(false, "igvn worklist must still be empty after verify"); } // Check that type(n) == n->Value(), return true if we have a failure. @@ -1104,7 +1132,7 @@ void PhaseIterGVN::verify_optimize() { // (1) Integer "widen" changes, but the range is the same. // (2) LoadNode performs deep traversals. Load is not notified for changes far away. // (3) CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away. -bool PhaseIterGVN::verify_node_value(Node* n) { +bool PhaseIterGVN::verify_Value_for(Node* n) { // If we assert inside type(n), because the type is still a null, then maybe // the node never went through gvn.transform, which would be a bug. const Type* told = type(n); @@ -1149,15 +1177,873 @@ bool PhaseIterGVN::verify_node_value(Node* n) { // after loop-opts, so that should take care of many of these cases. return false; } - tty->cr(); - tty->print_cr("Missed Value optimization:"); - n->dump_bfs(1, nullptr, ""); - tty->print_cr("Current type:"); - told->dump_on(tty); - tty->cr(); - tty->print_cr("Optimized type:"); - tnew->dump_on(tty); - tty->cr(); + + stringStream ss; // Print as a block without tty lock. + ss.cr(); + ss.print_cr("Missed Value optimization:"); + n->dump_bfs(1, nullptr, "", &ss); + ss.print_cr("Current type:"); + told->dump_on(&ss); + ss.cr(); + ss.print_cr("Optimized type:"); + tnew->dump_on(&ss); + ss.cr(); + tty->print_cr("%s", ss.as_string()); + return true; +} + +// Check that all Ideal optimizations that could be done were done. +// Returns true if it found missed optimization opportunities and +// false otherwise (no missed optimization, or skipped verification). +bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { + // First, we check a list of exceptions, where we skip verification, + // because there are known cases where Ideal can optimize after IGVN. + // Some may be expected and cannot be fixed, and others should be fixed. + switch (n->Opcode()) { + // RangeCheckNode::Ideal looks up the chain for about 999 nodes + // (see "Range-Check scan limit"). So, it is possible that something + // is optimized in that input subgraph, and the RangeCheck was not + // added to the worklist because it would be too expensive to walk + // down the graph for 1000 nodes and put all on the worklist. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xbatch --version + case Op_RangeCheck: + return false; + + // IfNode::Ideal does: + // Node* prev_dom = search_identical(dist, igvn); + // which means we seach up the CFG, traversing at most up to a distance. + // If anything happens rather far away from the If, we may not put the If + // back on the worklist. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_If: + return false; + + // IfNode::simple_subsuming + // Looks for dominating test that subsumes the current test. + // Notification could be difficult because of larger distance. + // + // Found with: + // runtime/exceptionMsgs/ArrayIndexOutOfBoundsException/ArrayIndexOutOfBoundsExceptionTest.java#id1 + // -XX:VerifyIterativeGVN=1110 + case Op_CountedLoopEnd: + return false; + + // LongCountedLoopEndNode::Ideal + // Probably same issue as above. + // + // Found with: + // compiler/predicates/assertion/TestAssertionPredicates.java#NoLoopPredicationXbatch + // -XX:StressLongCountedLoop=2000000 -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + case Op_LongCountedLoopEnd: + return false; + + // RegionNode::Ideal does "Skip around the useless IF diamond". + // 245 IfTrue === 244 + // 258 If === 245 257 + // 259 IfTrue === 258 [[ 263 ]] + // 260 IfFalse === 258 [[ 263 ]] + // 263 Region === 263 260 259 [[ 263 268 ]] + // to + // 245 IfTrue === 244 + // 263 Region === 263 245 _ [[ 263 268 ]] + // + // "Useless" means that there is no code in either branch of the If. + // I found a case where this was not done yet during IGVN. + // Why does the Region not get added to IGVN worklist when the If diamond becomes useless? + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_Region: + return false; + + // In AddNode::Ideal, we call "commute", which swaps the inputs so + // that smaller idx are first. Tracking it back, it led me to + // PhaseIdealLoop::remix_address_expressions which swapped the edges. + // + // Example: + // Before PhaseIdealLoop::remix_address_expressions + // 154 AddI === _ 12 144 + // After PhaseIdealLoop::remix_address_expressions + // 154 AddI === _ 144 12 + // After AddNode::Ideal + // 154 AddI === _ 12 144 + // + // I suspect that the node should be added to the IGVN worklist after + // PhaseIdealLoop::remix_address_expressions + // + // This is the only case I looked at, there may be others. Found like this: + // java -XX:VerifyIterativeGVN=0100 -Xbatch --version + // + // The following hit the same logic in PhaseIdealLoop::remix_address_expressions. + // + // Note: currently all of these fail also for other reasons, for example + // because of "commute" doing the reordering with the phi below. Once + // that is resolved, we can come back to this issue here. + // + // case Op_AddD: + // case Op_AddI: + // case Op_AddL: + // case Op_AddF: + // case Op_MulI: + // case Op_MulL: + // case Op_MulF: + // case Op_MulD: + // if (n->in(1)->_idx > n->in(2)->_idx) { + // // Expect "commute" to revert this case. + // return false; + // } + // break; // keep verifying + + // AddFNode::Ideal calls "commute", which can reorder the inputs for this: + // Check for tight loop increments: Loop-phi of Add of loop-phi + // It wants to take the phi into in(1): + // 471 Phi === 435 38 390 + // 390 AddF === _ 471 391 + // + // Other Associative operators are also affected equally. + // + // Investigate why this does not happen earlier during IGVN. + // + // Found with: + // test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java + // -XX:VerifyIterativeGVN=1110 + case Op_AddD: + //case Op_AddI: // Also affected for other reasons, see case further down. + //case Op_AddL: // Also affected for other reasons, see case further down. + case Op_AddF: + case Op_MulI: + case Op_MulL: + case Op_MulF: + case Op_MulD: + case Op_MinF: + case Op_MinD: + case Op_MaxF: + case Op_MaxD: + // XorINode::Ideal + // Found with: + // compiler/intrinsics/chacha/TestChaCha20.java + // -XX:VerifyIterativeGVN=1110 + case Op_XorI: + case Op_XorL: + // It seems we may have similar issues with the HF cases. + // Found with aarch64: + // compiler/vectorization/TestFloat16VectorOperations.java + // -XX:VerifyIterativeGVN=1110 + case Op_AddHF: + case Op_MulHF: + case Op_MaxHF: + case Op_MinHF: + return false; + + // In MulNode::Ideal the edges can be swapped to help value numbering: + // + // // We are OK if right is a constant, or right is a load and + // // left is a non-constant. + // if( !(t2->singleton() || + // (in(2)->is_Load() && !(t1->singleton() || in(1)->is_Load())) ) ) { + // if( t1->singleton() || // Left input is a constant? + // // Otherwise, sort inputs (commutativity) to help value numbering. + // (in(1)->_idx > in(2)->_idx) ) { + // swap_edges(1, 2); + // + // Why was this not done earlier during IGVN? + // + // Found with: + // test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithG1.java + // -XX:VerifyIterativeGVN=1110 + case Op_AndI: + // Same for AndL. + // Found with: + // compiler/intrinsics/bigInteger/MontgomeryMultiplyTest.java + // -XX:VerifyIterativeGVN=1110 + case Op_AndL: + return false; + + // SubLNode::Ideal does transform like: + // Convert "c1 - (y+c0)" into "(c1-c0) - y" + // + // In IGVN before verification: + // 8423 ConvI2L === _ 3519 [[ 8424 ]] #long:-2 + // 8422 ConvI2L === _ 8399 [[ 8424 ]] #long:3..256:www + // 8424 AddL === _ 8422 8423 [[ 8383 ]] !orig=[8382] + // 8016 ConL === 0 [[ 8383 ]] #long:0 + // 8383 SubL === _ 8016 8424 [[ 8156 ]] !orig=[8154] + // + // And then in verification: + // 8338 ConL === 0 [[ 8339 8424 ]] #long:-2 <----- Was constant folded. + // 8422 ConvI2L === _ 8399 [[ 8424 ]] #long:3..256:www + // 8424 AddL === _ 8422 8338 [[ 8383 ]] !orig=[8382] + // 8016 ConL === 0 [[ 8383 ]] #long:0 + // 8383 SubL === _ 8016 8424 [[ 8156 ]] !orig=[8154] + // + // So the form changed from: + // c1 - (y + [8423 ConvI2L]) + // to + // c1 - (y + -2) + // but the SubL was not added to the IGVN worklist. Investigate why. + // There could be other issues too. + // + // There seems to be a related AddL IGVN optimization that triggers + // the same SubL optimization, so investigate that too. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_SubL: + return false; + + // SubINode::Ideal does + // Convert "x - (y+c0)" into "(x-y) - c0" AND + // Convert "c1 - (y+c0)" into "(c1-c0) - y" + // + // Investigate why this does not yet happen during IGVN. + // + // Found with: + // test/hotspot/jtreg/compiler/c2/IVTest.java + // -XX:VerifyIterativeGVN=1110 + case Op_SubI: + return false; + + // AddNode::IdealIL does transform like: + // Convert x + (con - y) into "(x - y) + con" + // + // In IGVN before verification: + // 8382 ConvI2L + // 8381 ConvI2L === _ 791 [[ 8383 ]] #long:0 + // 8383 SubL === _ 8381 8382 + // 8168 ConvI2L + // 8156 AddL === _ 8168 8383 [[ 8158 ]] + // + // And then in verification: + // 8424 AddL + // 8016 ConL === 0 [[ 8383 ]] #long:0 <--- Was constant folded. + // 8383 SubL === _ 8016 8424 + // 8168 ConvI2L + // 8156 AddL === _ 8168 8383 [[ 8158 ]] + // + // So the form changed from: + // x + (ConvI2L(0) - [8382 ConvI2L]) + // to + // x + (0 - [8424 AddL]) + // but the AddL was not added to the IGVN worklist. Investigate why. + // There could be other issues, too. For example with "commute", see above. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_AddL: + return false; + + // SubTypeCheckNode::Ideal calls SubTypeCheckNode::verify_helper, which does + // Node* cmp = phase->transform(new CmpPNode(subklass, in(SuperKlass))); + // record_for_cleanup(cmp, phase); + // This verification code in the Ideal code creates new nodes, and checks + // if they fold in unexpected ways. This means some nodes are created and + // added to the worklist, even if the SubTypeCheck is not optimized. This + // goes agains the assumption of the verification here, which assumes that + // if the node is not optimized, then no new nodes should be created, and + // also no nodes should be added to the worklist. + // I see two options: + // 1) forbid what verify_helper does, because for each Ideal call it + // uses memory and that is suboptimal. But it is not clear how that + // verification can be done otherwise. + // 2) Special case the verification here. Probably the new nodes that + // were just created are dead, i.e. they are not connected down to + // root. We could verify that, and remove those nodes from the graph + // by setting all their inputs to nullptr. And of course we would + // have to remove those nodes from the worklist. + // Maybe there are other options too, I did not dig much deeper yet. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xbatch --version + case Op_SubTypeCheck: + return false; + + // LoopLimitNode::Ideal when stride is constant power-of-2, we can do a lowering + // to other nodes: Conv, Add, Sub, Mul, And ... + // + // 107 ConI === 0 [[ ... ]] #int:2 + // 84 LoadRange === _ 7 83 + // 50 ConI === 0 [[ ... ]] #int:0 + // 549 LoopLimit === _ 50 84 107 + // + // I stepped backward, to see how the node was generated, and I found that it was + // created in PhaseIdealLoop::exact_limit and not changed since. It is added to the + // IGVN worklist. I quickly checked when it goes into LoopLimitNode::Ideal after + // that, and it seems we want to skip lowering it until after loop-opts, but never + // add call record_for_post_loop_opts_igvn. This would be an easy fix, but there + // could be other issues too. + // + // Fond with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_LoopLimit: + return false; + + // PhiNode::Ideal calls split_flow_path, which tries to do this: + // "This optimization tries to find two or more inputs of phi with the same constant + // value. It then splits them into a separate Phi, and according Region." + // + // Example: + // 130 DecodeN === _ 129 + // 50 ConP === 0 [[ 18 91 99 18 ]] #null + // 18 Phi === 14 50 130 50 [[ 133 ]] #java/lang/Object * Oop:java/lang/Object * + // + // turns into: + // + // 50 ConP === 0 [[ 99 91 18 ]] #null + // 130 DecodeN === _ 129 [[ 18 ]] + // 18 Phi === 14 130 50 [[ 133 ]] #java/lang/Object * Oop:java/lang/Object * + // + // We would have to investigate why this optimization does not happen during IGVN. + // There could also be other issues - I did not investigate further yet. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_Phi: + return false; + + // MemBarNode::Ideal does "Eliminate volatile MemBars for scalar replaced objects". + // For examle "The allocated object does not escape". + // + // It seems the difference to earlier calls to MemBarNode::Ideal, is that there + // alloc->as_Allocate()->does_not_escape_thread() returned false, but in verification + // it returned true. Why does the MemBarStoreStore not get added to the IGVN + // worklist when this change happens? + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_MemBarStoreStore: + return false; + + // ConvI2LNode::Ideal converts + // 648 AddI === _ 583 645 [[ 661 ]] + // 661 ConvI2L === _ 648 [[ 664 ]] #long:0..maxint-1:www + // into + // 772 ConvI2L === _ 645 [[ 773 ]] #long:-120..maxint-61:www + // 771 ConvI2L === _ 583 [[ 773 ]] #long:60..120:www + // 773 AddL === _ 771 772 [[ ]] + // + // We have to investigate why this does not happen during IGVN in this case. + // There could also be other issues - I did not investigate further yet. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + case Op_ConvI2L: + return false; + + // AddNode::IdealIL can do this transform (and similar other ones): + // Convert "a*b+a*c into a*(b+c) + // The example had AddI(MulI(a, b), MulI(a, c)). Why did this not happen + // during IGVN? There was a mutation for one of the MulI, and only + // after that the pattern was as needed for the optimization. The MulI + // was added to the IGVN worklist, but not the AddI. This probably + // can be fixed by adding the correct pattern in add_users_of_use_to_worklist. + // + // Found with: + // test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java + // -XX:VerifyIterativeGVN=1110 + case Op_AddI: + return false; + + // ArrayCopyNode::Ideal + // calls ArrayCopyNode::prepare_array_copy + // calls Compile::conv_I2X_index -> is called with sizetype = intcon(0), I think that + // is not expected, and we create a range int:0..-1 + // calls Compile::constrained_convI2L -> creates ConvI2L(intcon(1), int:0..-1) + // note: the type is already empty! + // calls PhaseIterGVN::transform + // calls PhaseIterGVN::transform_old + // calls PhaseIterGVN::subsume_node -> subsume ConvI2L with TOP + // calls Unique_Node_List::push -> pushes TOP to worklist + // + // Once we get back to ArrayCopyNode::prepare_array_copy, we get back TOP, and + // return false. This means we eventually return nullptr from ArrayCopyNode::Ideal. + // + // Question: is it ok to push anything to the worklist during ::Ideal, if we will + // return nullptr, indicating nothing happened? + // Is it smart to do transform in Compile::constrained_convI2L, and then + // check for TOP in calls ArrayCopyNode::prepare_array_copy? + // Should we just allow TOP to land on the worklist, as an exception? + // + // Found with: + // compiler/arraycopy/TestArrayCopyAsLoadsStores.java + // -XX:VerifyIterativeGVN=1110 + case Op_ArrayCopy: + return false; + + // CastLLNode::Ideal + // calls ConstraintCastNode::optimize_integer_cast -> pushes CastLL through SubL + // + // Could be a notification issue, where updates inputs of CastLL do not notify + // down through SubL to CastLL. + // + // Found With: + // compiler/c2/TestMergeStoresMemorySegment.java#byte-array + // -XX:VerifyIterativeGVN=1110 + case Op_CastLL: + return false; + + // Similar case happens to CastII + // + // Found With: + // compiler/c2/TestScalarReplacementMaxLiveNodes.java + // -XX:VerifyIterativeGVN=1110 + case Op_CastII: + return false; + + // MaxLNode::Ideal + // calls AddNode::Ideal + // calls commute -> decides to swap edges + // + // Another notification issue, because we check inputs of inputs? + // MaxL -> Phi -> Loop + // MaxL -> Phi -> MaxL + // + // Found with: + // compiler/c2/irTests/TestIfMinMax.java + // -XX:VerifyIterativeGVN=1110 + case Op_MaxL: + case Op_MinL: + return false; + + // OrINode::Ideal + // calls AddNode::Ideal + // calls commute -> left is Load, right not -> commute. + // + // Not sure why notification does not work here, seems like + // the depth is only 1, so it should work. Needs investigation. + // + // Found with: + // compiler/codegen/TestCharVect2.java#id0 + // -XX:VerifyIterativeGVN=1110 + case Op_OrI: + case Op_OrL: + return false; + + // Bool -> constant folded to 1. + // Issue with notification? + // + // Found with: + // compiler/c2/irTests/TestVectorizationMismatchedAccess.java + // -XX:VerifyIterativeGVN=1110 + case Op_Bool: + return false; + + // LShiftLNode::Ideal + // Looks at pattern: "(x + x) << c0", converts it to "x << (c0 + 1)" + // Probably a notification issue. + // + // Found with: + // compiler/conversions/TestMoveConvI2LOrCastIIThruAddIs.java + // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + case Op_LShiftL: + return false; + + // LShiftINode::Ideal + // pattern: ((x + con1) << con2) -> x << con2 + con1 << con2 + // Could be issue with notification of inputs of inputs + // + // Side-note: should cases like these not be shared between + // LShiftI and LShiftL? + // + // Found with: + // compiler/escapeAnalysis/Test6689060.java + // -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + case Op_LShiftI: + return false; + + // AddPNode::Ideal seems to do set_req without removing lock first. + // Found with various vector tests tier1-tier3. + case Op_AddP: + return false; + + // StrIndexOfNode::Ideal + // Found in tier1-3. + case Op_StrIndexOf: + case Op_StrIndexOfChar: + return false; + + // StrEqualsNode::Identity + // + // Found (linux x64 only?) with: + // serviceability/sa/ClhsdbThreadContext.java + // -XX:+UnlockExperimentalVMOptions -XX:LockingMode=1 -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + case Op_StrEquals: + return false; + + // AryEqNode::Ideal + // Not investigated. Reshapes itself and adds lots of nodes to the worklist. + // + // Found with: + // vmTestbase/vm/mlvm/meth/stress/compiler/i2c_c2i/Test.java + // -XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -XX:+StressUnstableIfTraps -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + case Op_AryEq: + return false; + + // MergeMemNode::Ideal + // Found in tier1-3. Did not investigate further yet. + case Op_MergeMem: + return false; + + // URShiftINode::Ideal + // Found in tier1-3. Did not investigate further yet. + case Op_URShiftI: + return false; + + // CMoveINode::Ideal + // Found in tier1-3. Did not investigate further yet. + case Op_CMoveI: + return false; + + // CmpPNode::Ideal calls isa_const_java_mirror + // and generates new constant nodes, even if no progress is made. + // We can probably rewrite this so that only types are generated. + // It seems that object types are not hashed, we could investigate + // if that is an option as well. + // + // Found with: + // java -XX:VerifyIterativeGVN=1110 -Xcomp --version + case Op_CmpP: + return false; + + // MinINode::Ideal + // Did not investigate, but there are some patterns that might + // need more notification. + case Op_MinI: + case Op_MaxI: // preemptively removed it as well. + return false; + } + + if (n->is_Load()) { + // LoadNode::Ideal uses tries to find an earlier memory state, and + // checks can_see_stored_value for it. + // + // Investigate why this was not already done during IGVN. + // A similar issue happens with Identity. + // + // There seem to be other cases where loads go up some steps, like + // LoadNode::Ideal going up 10x steps to find dominating load. + // + // Found with: + // test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java + // -XX:VerifyIterativeGVN=1110 + return false; + } + + if (n->is_Store()) { + // StoreNode::Ideal can do this: + // // Capture an unaliased, unconditional, simple store into an initializer. + // // Or, if it is independent of the allocation, hoist it above the allocation. + // That replaces the Store with a MergeMem. + // + // We have to investigate why this does not happen during IGVN in this case. + // There could also be other issues - I did not investigate further yet. + // + // Found with: + // java -XX:VerifyIterativeGVN=0100 -Xcomp --version + return false; + } + + if (n->is_Vector()) { + // VectorNode::Ideal swaps edges, but only for ops + // that are deemed commutable. But swap_edges + // requires the hash to be invariant when the edges + // are swapped, which is not implemented for these + // vector nodes. This seems not to create any trouble + // usually, but we can also get graphs where in the + // end the nodes are not all commuted, so there is + // definitively an issue here. + // + // Probably we have two options: kill the hash, or + // properly make the hash commutation friendly. + // + // Found with: + // compiler/vectorapi/TestMaskedMacroLogicVector.java + // -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 -XX:+UseParallelGC -XX:+UseNUMA + return false; + } + + if (n->is_Region()) { + // LoopNode::Ideal calls RegionNode::Ideal. + // CountedLoopNode::Ideal calls RegionNode::Ideal too. + // But I got an issue because RegionNode::optimize_trichotomy + // then modifies another node, and pushes nodes to the worklist + // Not sure if this is ok, modifying another node like that. + // Maybe it is, then we need to look into what to do with + // the nodes that are now on the worklist, maybe just clear + // them out again. But maybe modifying other nodes like that + // is also bad design. In the end, we return nullptr for + // the current CountedLoop. But the extra nodes on the worklist + // trip the asserts later on. + // + // Found with: + // compiler/eliminateAutobox/TestShortBoxing.java + // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + return false; + } + + if (n->is_CallJava()) { + // CallStaticJavaNode::Ideal + // Led to a crash: + // assert((is_CallStaticJava() && cg->is_mh_late_inline()) || (is_CallDynamicJava() && cg->is_virtual_late_inline())) failed: mismatch + // + // Did not investigate yet, could be a bug. + // Or maybe it does not expect to be called during verification. + // + // Found with: + // test/jdk/jdk/incubator/vector/VectorRuns.java + // -XX:VerifyIterativeGVN=1110 + + // CallDynamicJavaNode::Ideal, and I think also for CallStaticJavaNode::Ideal + // and possibly their subclasses. + // During late inlining it can call CallJavaNode::register_for_late_inline + // That means we do more rounds of late inlining, but might fail. + // Then we do IGVN again, and register the node again for late inlining. + // This creates an endless cycle. Everytime we try late inlining, we + // are also creating more nodes, especially SafePoint and MergeMem. + // These nodes are immediately rejected when the inlining fails in the + // do_late_inline_check, but they still grow the memory, until we hit + // the MemLimit and crash. + // The assumption here seems that CallDynamicJavaNode::Ideal does not get + // called repeatedly, and eventually we terminate. I fear this is not + // a great assumption to make. We should investigate more. + // + // Found with: + // compiler/loopopts/superword/TestDependencyOffsets.java#vanilla-U + // -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + return false; + } + + // The number of nodes shoud not increase. + uint old_unique = C->unique(); + + Node* i = n->Ideal(this, can_reshape); + // If there was no new Idealization, we are probably happy. + if (i == nullptr) { + if (old_unique < C->unique()) { + stringStream ss; // Print as a block without tty lock. + ss.cr(); + ss.print_cr("Ideal optimization did not make progress but created new unused nodes."); + ss.print_cr(" old_unique = %d, unique = %d", old_unique, C->unique()); + n->dump_bfs(1, nullptr, "", &ss); + tty->print_cr("%s", ss.as_string()); + return true; + } + + verify_empty_worklist(n); + + // Everything is good. + return false; + } + + // We just saw a new Idealization which was not done during IGVN. + stringStream ss; // Print as a block without tty lock. + ss.cr(); + ss.print_cr("Missed Ideal optimization (can_reshape=%s):", can_reshape ? "true": "false"); + if (i == n) { + ss.print_cr("The node was reshaped by Ideal."); + } else { + ss.print_cr("The node was replaced by Ideal."); + ss.print_cr("Old node:"); + n->dump_bfs(1, nullptr, "", &ss); + } + ss.print_cr("The result after Ideal:"); + i->dump_bfs(1, nullptr, "", &ss); + tty->print_cr("%s", ss.as_string()); + return true; +} + +// Check that all Identity optimizations that could be done were done. +// Returns true if it found missed optimization opportunities and +// false otherwise (no missed optimization, or skipped verification). +bool PhaseIterGVN::verify_Identity_for(Node* n) { + // First, we check a list of exceptions, where we skip verification, + // because there are known cases where Ideal can optimize after IGVN. + // Some may be expected and cannot be fixed, and others should be fixed. + switch (n->Opcode()) { + // SafePointNode::Identity can remove SafePoints, but wants to wait until + // after loopopts: + // // Transforming long counted loops requires a safepoint node. Do not + // // eliminate a safepoint until loop opts are over. + // if (in(0)->is_Proj() && !phase->C->major_progress()) { + // + // I think the check for major_progress does delay it until after loopopts + // but it does not ensure that the node is on the IGVN worklist after + // loopopts. I think we should try to instead check for + // phase->C->post_loop_opts_phase() and call record_for_post_loop_opts_igvn. + // + // Found with: + // java -XX:VerifyIterativeGVN=1000 -Xcomp --version + case Op_SafePoint: + return false; + + // MergeMemNode::Identity replaces the MergeMem with its base_memory if it + // does not record any other memory splits. + // + // I did not deeply investigate, but it looks like MergeMemNode::Identity + // never got called during IGVN for this node, investigate why. + // + // Found with: + // java -XX:VerifyIterativeGVN=1000 -Xcomp --version + case Op_MergeMem: + return false; + + // ConstraintCastNode::Identity finds casts that are the same, except that + // the control is "higher up", i.e. dominates. The call goes via + // ConstraintCastNode::dominating_cast to PhaseGVN::is_dominator_helper, + // which traverses up to 100 idom steps. If anything gets optimized somewhere + // away from the cast, but within 100 idom steps, the cast may not be + // put on the IGVN worklist any more. + // + // Found with: + // java -XX:VerifyIterativeGVN=1000 -Xcomp --version + case Op_CastPP: + case Op_CastII: + case Op_CastLL: + return false; + + // Same issue for CheckCastPP, uses ConstraintCastNode::Identity and + // checks dominator, which may be changed, but too far up for notification + // to work. + // + // Found with: + // compiler/c2/irTests/TestSkeletonPredicates.java + // -XX:VerifyIterativeGVN=1110 + case Op_CheckCastPP: + return false; + + // In SubNode::Identity, we do: + // Convert "(X+Y) - Y" into X and "(X+Y) - X" into Y + // In the example, the AddI had an input replaced, the AddI is + // added to the IGVN worklist, but the SubI is one link further + // down and is not added. I checked add_users_of_use_to_worklist + // where I would expect the SubI would be added, and I cannot + // find the pattern, only this one: + // If changed AddI/SubI inputs, check CmpU for range check optimization. + // + // Fix this "notification" issue and check if there are any other + // issues. + // + // Found with: + // java -XX:VerifyIterativeGVN=1000 -Xcomp --version + case Op_SubI: + case Op_SubL: + return false; + + // PhiNode::Identity checks for patterns like: + // r = (x != con) ? x : con; + // that can be constant folded to "x". + // + // Call goes through PhiNode::is_cmove_id and CMoveNode::is_cmove_id. + // I suspect there was some earlier change to one of the inputs, but + // not all relevant outputs were put on the IGVN worklist. + // + // Found with: + // test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithG1.java + // -XX:VerifyIterativeGVN=1110 + case Op_Phi: + return false; + + // ConvI2LNode::Identity does + // convert I2L(L2I(x)) => x + // + // Investigate why this did not already happen during IGVN. + // + // Found with: + // compiler/loopopts/superword/TestDependencyOffsets.java#vanilla-A + // -XX:VerifyIterativeGVN=1110 + case Op_ConvI2L: + return false; + + // MaxNode::find_identity_operation + // Finds patterns like Max(A, Max(A, B)) -> Max(A, B) + // This can be a 2-hop search, so maybe notification is not + // good enough. + // + // Found with: + // compiler/codegen/TestBooleanVect.java + // -XX:VerifyIterativeGVN=1110 + case Op_MaxL: + case Op_MinL: + case Op_MaxI: + case Op_MinI: + case Op_MaxF: + case Op_MinF: + case Op_MaxHF: + case Op_MinHF: + case Op_MaxD: + case Op_MinD: + return false; + + + // AddINode::Identity + // Converts (x-y)+y to x + // Could be issue with notification + // + // Turns out AddL does the same. + // + // Found with: + // compiler/c2/Test6792161.java + // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + case Op_AddI: + case Op_AddL: + return false; + + // AbsINode::Identity + // Not investigated yet. + case Op_AbsI: + return false; + } + + if (n->is_Load()) { + // LoadNode::Identity tries to look for an earlier store value via + // can_see_stored_value. I found an example where this led to + // an Allocation, where we could assume the value was still zero. + // So the LoadN can be replaced with a zerocon. + // + // Investigate why this was not already done during IGVN. + // A similar issue happens with Ideal. + // + // Found with: + // java -XX:VerifyIterativeGVN=1000 -Xcomp --version + return false; + } + + if (n->is_Store()) { + // StoreNode::Identity + // Not investigated, but found missing optimization for StoreI. + // Looks like a StoreI is replaced with an InitializeNode. + // + // Found with: + // applications/ctw/modules/java_base_2.java + // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -Djava.awt.headless=true -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + return false; + } + + if (n->is_Vector()) { + // Found with tier1-3. Not investigated yet. + // The observed issue was with AndVNode::Identity + return false; + } + + Node* i = n->Identity(this); + // If we cannot find any other Identity, we are happy. + if (i == n) { + verify_empty_worklist(n); + return false; + } + + // The verification just found a new Identity that was not found during IGVN. + stringStream ss; // Print as a block without tty lock. + ss.cr(); + ss.print_cr("Missed Identity optimization:"); + ss.print_cr("Old node:"); + n->dump_bfs(1, nullptr, "", &ss); + ss.print_cr("New node:"); + i->dump_bfs(1, nullptr, "", &ss); + tty->print_cr("%s", ss.as_string()); return true; } #endif @@ -1892,12 +2778,12 @@ void PhaseCCP::analyze() { #ifdef ASSERT // For every node n on verify list, check if type(n) == n->Value() -// We have a list of exceptions, see comments in verify_node_value. +// We have a list of exceptions, see comments in verify_Value_for. void PhaseCCP::verify_analyze(Unique_Node_List& worklist_verify) { bool failure = false; while (worklist_verify.size()) { Node* n = worklist_verify.pop(); - failure |= verify_node_value(n); + failure |= verify_Value_for(n); } // If we get this assert, check why the reported nodes were not processed again in CCP. // We should either make sure that these nodes are properly added back to the CCP worklist diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index af39de9c002..aeba5e8662d 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2025, Oracle and/or its affiliates. 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 @@ -494,7 +494,10 @@ class PhaseIterGVN : public PhaseGVN { void optimize(); #ifdef ASSERT void verify_optimize(); - bool verify_node_value(Node* n); + bool verify_Value_for(Node* n); + bool verify_Ideal_for(Node* n, bool can_reshape); + bool verify_Identity_for(Node* n); + void verify_empty_worklist(Node* n); #endif #ifndef PRODUCT @@ -593,6 +596,14 @@ class PhaseIterGVN : public PhaseGVN { // '-XX:VerifyIterativeGVN=10' return ((VerifyIterativeGVN % 100) / 10) == 1; } + static bool is_verify_Ideal() { + // '-XX:VerifyIterativeGVN=100' + return ((VerifyIterativeGVN % 1000) / 100) == 1; + } + static bool is_verify_Identity() { + // '-XX:VerifyIterativeGVN=1000' + return ((VerifyIterativeGVN % 10000) / 1000) == 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 d9972b21ea1..18aa4a56d71 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp @@ -299,8 +299,9 @@ JVMFlag::Error TypeProfileLevelConstraintFunc(uint value, bool verbose) { } JVMFlag::Error VerifyIterativeGVNConstraintFunc(uint value, bool verbose) { + const int max_modes = 4; uint original_value = value; - for (int i = 0; i < 2; i++) { + for (int i = 0; i < max_modes; i++) { if (value % 10 > 1) { JVMFlag::printError(verbose, "Invalid value (" UINT32_FORMAT ") " @@ -312,7 +313,7 @@ JVMFlag::Error VerifyIterativeGVNConstraintFunc(uint value, bool verbose) { if (value != 0) { JVMFlag::printError(verbose, "Invalid value (" UINT32_FORMAT ") " - "for VerifyIterativeGVN: maximal 2 digits\n", original_value); + "for VerifyIterativeGVN: maximal %d digits\n", original_value, max_modes); return JVMFlag::VIOLATES_CONSTRAINT; } return JVMFlag::SUCCESS; diff --git a/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java b/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java index f3f589f8fb1..83f3540226f 100644 --- a/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java +++ b/test/hotspot/jtreg/compiler/c2/TestVerifyIterativeGVN.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2020, 2025, Oracle and/or its affiliates. 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 @@ -25,9 +25,9 @@ * @test * @bug 8238756 * @requires vm.debug == true & vm.flavor == "server" - * @summary Run with -Xcomp to test -XX:VerifyIterativeGVN=11 in debug builds. + * @summary Run with -Xcomp to test -XX:VerifyIterativeGVN=1111 in debug builds. * - * @run main/othervm/timeout=300 -Xbatch -Xcomp -XX:VerifyIterativeGVN=11 compiler.c2.TestVerifyIterativeGVN + * @run main/othervm/timeout=300 -Xcomp -XX:VerifyIterativeGVN=1111 compiler.c2.TestVerifyIterativeGVN */ package compiler.c2; From 9439aa565f36f6db50e5041c4de45e0659f66965 Mon Sep 17 00:00:00 2001 From: Roland Westrelin Date: Mon, 15 Dec 2025 16:18:44 +0000 Subject: [PATCH 4/7] 8351889: C2 crash: assertion failed: Base pointers must match (addp 344) Reviewed-by: rcastanedalo, epeter --- src/hotspot/share/opto/addnode.hpp | 7 ++ src/hotspot/share/opto/c2_globals.hpp | 3 +- src/hotspot/share/opto/cfgnode.cpp | 36 +++++++++ src/hotspot/share/opto/cfgnode.hpp | 2 + src/hotspot/share/opto/compile.cpp | 5 +- src/hotspot/share/opto/phaseX.cpp | 21 ++++- src/hotspot/share/opto/phaseX.hpp | 5 ++ .../flags/jvmFlagConstraintsCompiler.cpp | 2 +- .../c2/TestMismatchedAddPAfterMaxUnroll.java | 80 +++++++++++++++++++ .../compiler/c2/TestVerifyIterativeGVN.java | 6 +- 10 files changed, 156 insertions(+), 11 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/c2/TestMismatchedAddPAfterMaxUnroll.java diff --git a/src/hotspot/share/opto/addnode.hpp b/src/hotspot/share/opto/addnode.hpp index 456a8d9f9a0..d2e13de6c68 100644 --- a/src/hotspot/share/opto/addnode.hpp +++ b/src/hotspot/share/opto/addnode.hpp @@ -201,6 +201,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 227817612ce..4160ac8555b 100644 --- a/src/hotspot/share/opto/c2_globals.hpp +++ b/src/hotspot/share/opto/c2_globals.hpp @@ -678,7 +678,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 cdcfe3a6b35..cdade0b7f0b 100644 --- a/src/hotspot/share/opto/cfgnode.cpp +++ b/src/hotspot/share/opto/cfgnode.cpp @@ -2095,6 +2095,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. @@ -2153,6 +2167,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 b17326bb511..3850bae8e2d 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 2b956dcb5d8..0c341c245b5 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -3383,10 +3383,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 c88ac0c3984..2e7a0012725 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); @@ -2046,6 +2048,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 aeba5e8662d..e436a61fa73 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -497,6 +497,7 @@ class PhaseIterGVN : public PhaseGVN { bool verify_Value_for(Node* n); 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 @@ -604,6 +605,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 18aa4a56d71..3c7d6e5e11c 100644 --- a/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp +++ b/src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp @@ -299,7 +299,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 00000000000..2dcafd2cf6a --- /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 83f3540226f..4b6215b25bd 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; From af6ff9a2ed4bdb57beaba2a4be330bff4f714279 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Maillard?= Date: Mon, 14 Jul 2025 11:40:00 +0000 Subject: [PATCH 5/7] 8361144: Strenghten the Ideal Verification in PhaseIterGVN::verify_Ideal_for by comparing the hash of a node before and after Ideal Co-authored-by: Emanuel Peter Reviewed-by: galder, dfenacci, epeter --- src/hotspot/share/opto/phaseX.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index 2e7a0012725..dacd3acf122 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1820,7 +1820,8 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // The number of nodes shoud not increase. uint old_unique = C->unique(); - + // The hash of a node should not change, this would indicate different inputs + uint old_hash = n->hash(); Node* i = n->Ideal(this, can_reshape); // If there was no new Idealization, we are probably happy. if (i == nullptr) { @@ -1834,6 +1835,16 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { return true; } + if (old_hash != n->hash()) { + stringStream ss; // Print as a block without tty lock. + ss.cr(); + ss.print_cr("Ideal optimization did not make progress but node hash changed."); + ss.print_cr(" old_hash = %d, hash = %d", old_hash, n->hash()); + n->dump_bfs(1, nullptr, "", &ss); + tty->print_cr("%s", ss.as_string()); + return true; + } + verify_empty_worklist(n); // Everything is good. From 5b23f61a0e5a4fd4252dcd455b0611f43e0ec188 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Maillard?= Date: Mon, 2 Feb 2026 10:10:21 +0000 Subject: [PATCH 6/7] 8371536: C2: VerifyIterativeGVN should assert on first detected failure Reviewed-by: epeter, mhaessig, chagedorn --- src/hotspot/share/opto/phaseX.cpp | 222 +++++++++++++++++------------- src/hotspot/share/opto/phaseX.hpp | 20 ++- 2 files changed, 141 insertions(+), 101 deletions(-) diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index dacd3acf122..d82bc34b847 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -532,6 +532,10 @@ void PhaseValues::init_con_caches() { memset(_zcons,0,sizeof(_zcons)); } +PhaseIterGVN* PhaseValues::is_IterGVN() { + return (_phase == PhaseValuesType::iter_gvn || _phase == PhaseValuesType::ccp) ? static_cast(this) : nullptr; +} + //--------------------------------find_int_type-------------------------------- const TypeInt* PhaseValues::find_int_type(Node* n) { if (n == nullptr) return nullptr; @@ -802,7 +806,7 @@ void PhaseGVN::dump_infinite_loop_info(Node* n, const char* where) { PhaseIterGVN::PhaseIterGVN(PhaseIterGVN* igvn) : _delay_transform(igvn->_delay_transform), _worklist(*C->igvn_worklist()) { - _iterGVN = true; + _phase = PhaseValuesType::iter_gvn; assert(&_worklist == &igvn->_worklist, "sanity"); } @@ -811,7 +815,7 @@ PhaseIterGVN::PhaseIterGVN(PhaseIterGVN* igvn) : _delay_transform(igvn->_delay_t PhaseIterGVN::PhaseIterGVN(PhaseGVN* gvn) : _delay_transform(false), _worklist(*C->igvn_worklist()) { - _iterGVN = true; + _phase = PhaseValuesType::iter_gvn; uint max; // Dead nodes in the hash table inherited from GVN were not treated as @@ -1080,16 +1084,28 @@ void PhaseIterGVN::verify_optimize() { is_verify_invariants()) { ResourceMark rm; Unique_Node_List worklist; - bool failure = false; // BFS all nodes, starting at root worklist.push(C->root()); for (uint j = 0; j < worklist.size(); ++j) { Node* n = worklist.at(j); - if (is_verify_Value()) { failure |= verify_Value_for(n); } - 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); } + // If we get an assert here, check why the reported node was not processed again in IGVN. + // We should either make sure that this node is properly added back to the IGVN worklist + // in PhaseIterGVN::add_users_to_worklist to update it again or add an exception + // in the verification methods below if that is not possible for some reason (like Load nodes). + if (is_verify_Value()) { + verify_Value_for(n); + } + if (is_verify_Ideal()) { + verify_Ideal_for(n, false); + verify_Ideal_for(n, true); + } + if (is_verify_Identity()) { + verify_Identity_for(n); + } + if (is_verify_invariants()) { + verify_node_invariants_for(n); + } + // traverse all inputs and outputs for (uint i = 0; i < n->req(); i++) { if (n->in(i) != nullptr) { @@ -1100,11 +1116,6 @@ void PhaseIterGVN::verify_optimize() { worklist.push(n->fast_out(i)); } } - // If we get this assert, check why the reported nodes were not processed again in IGVN. - // 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/broken graph in PhaseIterGVN"); } verify_empty_worklist(nullptr); @@ -1129,18 +1140,18 @@ void PhaseIterGVN::verify_empty_worklist(Node* node) { assert(false, "igvn worklist must still be empty after verify"); } -// Check that type(n) == n->Value(), return true if we have a failure. +// Check that type(n) == n->Value(), asserts if we have a failure. // We have a list of exceptions, see detailed comments in code. // (1) Integer "widen" changes, but the range is the same. // (2) LoadNode performs deep traversals. Load is not notified for changes far away. // (3) CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away. -bool PhaseIterGVN::verify_Value_for(Node* n) { +void PhaseIterGVN::verify_Value_for(const Node* n, bool strict) { // If we assert inside type(n), because the type is still a null, then maybe // the node never went through gvn.transform, which would be a bug. const Type* told = type(n); const Type* tnew = n->Value(this); if (told == tnew) { - return false; + return; } // Exception (1) // Integer "widen" changes, but range is the same. @@ -1149,7 +1160,7 @@ bool PhaseIterGVN::verify_Value_for(Node* n) { const TypeInteger* t1 = tnew->is_integer(tnew->basic_type()); if (t0->lo_as_long() == t1->lo_as_long() && t0->hi_as_long() == t1->hi_as_long()) { - return false; // ignore integer widen + return; // ignore integer widen } } // Exception (2) @@ -1158,7 +1169,7 @@ bool PhaseIterGVN::verify_Value_for(Node* n) { // MemNode::can_see_stored_value looks up through many memory nodes, // which means we would need to notify modifications from far up in // the inputs all the way down to the LoadNode. We don't do that. - return false; + return; } // Exception (3) // CmpPNode performs deep traversals if it compares oopptr. CmpP is not notified for changes far away. @@ -1174,10 +1185,10 @@ bool PhaseIterGVN::verify_Value_for(Node* n) { // control sub of the allocation. The problems is that sometimes dominates answers // false conservatively, and later it can determine that it is indeed true. Loops with // Region heads can lead to giving up, whereas LoopNodes can be skipped easier, and - // so the traversal becomes more powerful. This is difficult to remidy, we would have + // so the traversal becomes more powerful. This is difficult to remedy, we would have // to notify the CmpP of CFG updates. Luckily, we recompute CmpP::Value during CCP // after loop-opts, so that should take care of many of these cases. - return false; + return; } stringStream ss; // Print as a block without tty lock. @@ -1191,13 +1202,24 @@ bool PhaseIterGVN::verify_Value_for(Node* n) { tnew->dump_on(&ss); ss.cr(); tty->print_cr("%s", ss.as_string()); - return true; + + switch (_phase) { + case PhaseValuesType::iter_gvn: + assert(false, "Missed Value optimization opportunity in PhaseIterGVN for %s",n->Name()); + break; + case PhaseValuesType::ccp: + assert(false, "PhaseCCP not at fixpoint: analysis result may be unsound for %s", n->Name()); + break; + default: + assert(false, "Unexpected phase"); + break; + } } // Check that all Ideal optimizations that could be done were done. -// Returns true if it found missed optimization opportunities and -// false otherwise (no missed optimization, or skipped verification). -bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { +// Asserts if it found missed optimization opportunities or encountered unexpected changes, and +// returns normally otherwise (no missed optimization, or skipped verification). +void PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // First, we check a list of exceptions, where we skip verification, // because there are known cases where Ideal can optimize after IGVN. // Some may be expected and cannot be fixed, and others should be fixed. @@ -1211,7 +1233,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xbatch --version case Op_RangeCheck: - return false; + return; // IfNode::Ideal does: // Node* prev_dom = search_identical(dist, igvn); @@ -1222,7 +1244,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_If: - return false; + return; // IfNode::simple_subsuming // Looks for dominating test that subsumes the current test. @@ -1232,7 +1254,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // runtime/exceptionMsgs/ArrayIndexOutOfBoundsException/ArrayIndexOutOfBoundsExceptionTest.java#id1 // -XX:VerifyIterativeGVN=1110 case Op_CountedLoopEnd: - return false; + return; // LongCountedLoopEndNode::Ideal // Probably same issue as above. @@ -1241,7 +1263,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/predicates/assertion/TestAssertionPredicates.java#NoLoopPredicationXbatch // -XX:StressLongCountedLoop=2000000 -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 case Op_LongCountedLoopEnd: - return false; + return; // RegionNode::Ideal does "Skip around the useless IF diamond". // 245 IfTrue === 244 @@ -1260,7 +1282,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_Region: - return false; + return; // In AddNode::Ideal, we call "commute", which swaps the inputs so // that smaller idx are first. Tracking it back, it led me to @@ -1339,7 +1361,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { case Op_MulHF: case Op_MaxHF: case Op_MinHF: - return false; + return; // In MulNode::Ideal the edges can be swapped to help value numbering: // @@ -1363,7 +1385,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/intrinsics/bigInteger/MontgomeryMultiplyTest.java // -XX:VerifyIterativeGVN=1110 case Op_AndL: - return false; + return; // SubLNode::Ideal does transform like: // Convert "c1 - (y+c0)" into "(c1-c0) - y" @@ -1395,7 +1417,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_SubL: - return false; + return; // SubINode::Ideal does // Convert "x - (y+c0)" into "(x-y) - c0" AND @@ -1407,7 +1429,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // test/hotspot/jtreg/compiler/c2/IVTest.java // -XX:VerifyIterativeGVN=1110 case Op_SubI: - return false; + return; // AddNode::IdealIL does transform like: // Convert x + (con - y) into "(x - y) + con" @@ -1436,7 +1458,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_AddL: - return false; + return; // SubTypeCheckNode::Ideal calls SubTypeCheckNode::verify_helper, which does // Node* cmp = phase->transform(new CmpPNode(subklass, in(SuperKlass))); @@ -1461,7 +1483,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xbatch --version case Op_SubTypeCheck: - return false; + return; // LoopLimitNode::Ideal when stride is constant power-of-2, we can do a lowering // to other nodes: Conv, Add, Sub, Mul, And ... @@ -1481,7 +1503,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Fond with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_LoopLimit: - return false; + return; // PhiNode::Ideal calls split_flow_path, which tries to do this: // "This optimization tries to find two or more inputs of phi with the same constant @@ -1504,7 +1526,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_Phi: - return false; + return; // MemBarNode::Ideal does "Eliminate volatile MemBars for scalar replaced objects". // For examle "The allocated object does not escape". @@ -1517,7 +1539,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_MemBarStoreStore: - return false; + return; // ConvI2LNode::Ideal converts // 648 AddI === _ 583 645 [[ 661 ]] @@ -1533,7 +1555,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version case Op_ConvI2L: - return false; + return; // AddNode::IdealIL can do this transform (and similar other ones): // Convert "a*b+a*c into a*(b+c) @@ -1547,7 +1569,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // test/hotspot/jtreg/compiler/loopopts/superword/ReductionPerf.java // -XX:VerifyIterativeGVN=1110 case Op_AddI: - return false; + return; // ArrayCopyNode::Ideal // calls ArrayCopyNode::prepare_array_copy @@ -1573,7 +1595,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/arraycopy/TestArrayCopyAsLoadsStores.java // -XX:VerifyIterativeGVN=1110 case Op_ArrayCopy: - return false; + return; // CastLLNode::Ideal // calls ConstraintCastNode::optimize_integer_cast -> pushes CastLL through SubL @@ -1585,7 +1607,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/c2/TestMergeStoresMemorySegment.java#byte-array // -XX:VerifyIterativeGVN=1110 case Op_CastLL: - return false; + return; // Similar case happens to CastII // @@ -1593,7 +1615,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/c2/TestScalarReplacementMaxLiveNodes.java // -XX:VerifyIterativeGVN=1110 case Op_CastII: - return false; + return; // MaxLNode::Ideal // calls AddNode::Ideal @@ -1608,7 +1630,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // -XX:VerifyIterativeGVN=1110 case Op_MaxL: case Op_MinL: - return false; + return; // OrINode::Ideal // calls AddNode::Ideal @@ -1622,7 +1644,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // -XX:VerifyIterativeGVN=1110 case Op_OrI: case Op_OrL: - return false; + return; // Bool -> constant folded to 1. // Issue with notification? @@ -1631,7 +1653,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/c2/irTests/TestVectorizationMismatchedAccess.java // -XX:VerifyIterativeGVN=1110 case Op_Bool: - return false; + return; // LShiftLNode::Ideal // Looks at pattern: "(x + x) << c0", converts it to "x << (c0 + 1)" @@ -1641,7 +1663,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/conversions/TestMoveConvI2LOrCastIIThruAddIs.java // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 case Op_LShiftL: - return false; + return; // LShiftINode::Ideal // pattern: ((x + con1) << con2) -> x << con2 + con1 << con2 @@ -1654,18 +1676,18 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // compiler/escapeAnalysis/Test6689060.java // -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 case Op_LShiftI: - return false; + return; // AddPNode::Ideal seems to do set_req without removing lock first. // Found with various vector tests tier1-tier3. case Op_AddP: - return false; + return; // StrIndexOfNode::Ideal // Found in tier1-3. case Op_StrIndexOf: case Op_StrIndexOfChar: - return false; + return; // StrEqualsNode::Identity // @@ -1673,7 +1695,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // serviceability/sa/ClhsdbThreadContext.java // -XX:+UnlockExperimentalVMOptions -XX:LockingMode=1 -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 case Op_StrEquals: - return false; + return; // AryEqNode::Ideal // Not investigated. Reshapes itself and adds lots of nodes to the worklist. @@ -1682,22 +1704,22 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // vmTestbase/vm/mlvm/meth/stress/compiler/i2c_c2i/Test.java // -XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -XX:+StressUnstableIfTraps -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 case Op_AryEq: - return false; + return; // MergeMemNode::Ideal // Found in tier1-3. Did not investigate further yet. case Op_MergeMem: - return false; + return; // URShiftINode::Ideal // Found in tier1-3. Did not investigate further yet. case Op_URShiftI: - return false; + return; // CMoveINode::Ideal // Found in tier1-3. Did not investigate further yet. case Op_CMoveI: - return false; + return; // CmpPNode::Ideal calls isa_const_java_mirror // and generates new constant nodes, even if no progress is made. @@ -1708,14 +1730,14 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // java -XX:VerifyIterativeGVN=1110 -Xcomp --version case Op_CmpP: - return false; + return; // MinINode::Ideal // Did not investigate, but there are some patterns that might // need more notification. case Op_MinI: case Op_MaxI: // preemptively removed it as well. - return false; + return; } if (n->is_Load()) { @@ -1731,7 +1753,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java // -XX:VerifyIterativeGVN=1110 - return false; + return; } if (n->is_Store()) { @@ -1745,7 +1767,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // // Found with: // java -XX:VerifyIterativeGVN=0100 -Xcomp --version - return false; + return; } if (n->is_Vector()) { @@ -1764,7 +1786,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // compiler/vectorapi/TestMaskedMacroLogicVector.java // -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 -XX:+UseParallelGC -XX:+UseNUMA - return false; + return; } if (n->is_Region()) { @@ -1783,7 +1805,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // compiler/eliminateAutobox/TestShortBoxing.java // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 - return false; + return; } if (n->is_CallJava()) { @@ -1815,13 +1837,19 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { // Found with: // compiler/loopopts/superword/TestDependencyOffsets.java#vanilla-U // -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 - return false; + return; } // The number of nodes shoud not increase. uint old_unique = C->unique(); // The hash of a node should not change, this would indicate different inputs uint old_hash = n->hash(); + // Remove 'n' from hash table in case it gets modified. We want to avoid + // hitting the "Need to remove from hash before changing edges" assert if + // a change occurs. Instead, we would like to proceed with the optimization, + // return and finally hit the assert in PhaseIterGVN::verify_optimize to get + // a more meaningful message + _table.hash_delete(n); Node* i = n->Ideal(this, can_reshape); // If there was no new Idealization, we are probably happy. if (i == nullptr) { @@ -1832,7 +1860,7 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { ss.print_cr(" old_unique = %d, unique = %d", old_unique, C->unique()); n->dump_bfs(1, nullptr, "", &ss); tty->print_cr("%s", ss.as_string()); - return true; + assert(false, "Unexpected new unused nodes from applying Ideal optimization on %s", n->Name()); } if (old_hash != n->hash()) { @@ -1842,13 +1870,14 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { ss.print_cr(" old_hash = %d, hash = %d", old_hash, n->hash()); n->dump_bfs(1, nullptr, "", &ss); tty->print_cr("%s", ss.as_string()); - return true; + assert(false, "Unexpected hash change from applying Ideal optimization on %s", n->Name()); } verify_empty_worklist(n); // Everything is good. - return false; + hash_find_insert(n); + return; } // We just saw a new Idealization which was not done during IGVN. @@ -1865,13 +1894,14 @@ bool PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { ss.print_cr("The result after Ideal:"); i->dump_bfs(1, nullptr, "", &ss); tty->print_cr("%s", ss.as_string()); - return true; + + assert(false, "Missed Ideal optimization opportunity in PhaseIterGVN for %s", n->Name()); } // Check that all Identity optimizations that could be done were done. -// Returns true if it found missed optimization opportunities and -// false otherwise (no missed optimization, or skipped verification). -bool PhaseIterGVN::verify_Identity_for(Node* n) { +// Asserts if it found missed optimization opportunities, and +// returns normally otherwise (no missed optimization, or skipped verification). +void PhaseIterGVN::verify_Identity_for(Node* n) { // First, we check a list of exceptions, where we skip verification, // because there are known cases where Ideal can optimize after IGVN. // Some may be expected and cannot be fixed, and others should be fixed. @@ -1890,7 +1920,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // Found with: // java -XX:VerifyIterativeGVN=1000 -Xcomp --version case Op_SafePoint: - return false; + return; // MergeMemNode::Identity replaces the MergeMem with its base_memory if it // does not record any other memory splits. @@ -1901,7 +1931,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // Found with: // java -XX:VerifyIterativeGVN=1000 -Xcomp --version case Op_MergeMem: - return false; + return; // ConstraintCastNode::Identity finds casts that are the same, except that // the control is "higher up", i.e. dominates. The call goes via @@ -1915,7 +1945,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { case Op_CastPP: case Op_CastII: case Op_CastLL: - return false; + return; // Same issue for CheckCastPP, uses ConstraintCastNode::Identity and // checks dominator, which may be changed, but too far up for notification @@ -1925,7 +1955,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // compiler/c2/irTests/TestSkeletonPredicates.java // -XX:VerifyIterativeGVN=1110 case Op_CheckCastPP: - return false; + return; // In SubNode::Identity, we do: // Convert "(X+Y) - Y" into X and "(X+Y) - X" into Y @@ -1943,7 +1973,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // java -XX:VerifyIterativeGVN=1000 -Xcomp --version case Op_SubI: case Op_SubL: - return false; + return; // PhiNode::Identity checks for patterns like: // r = (x != con) ? x : con; @@ -1957,7 +1987,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithG1.java // -XX:VerifyIterativeGVN=1110 case Op_Phi: - return false; + return; // ConvI2LNode::Identity does // convert I2L(L2I(x)) => x @@ -1968,7 +1998,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // compiler/loopopts/superword/TestDependencyOffsets.java#vanilla-A // -XX:VerifyIterativeGVN=1110 case Op_ConvI2L: - return false; + return; // MaxNode::find_identity_operation // Finds patterns like Max(A, Max(A, B)) -> Max(A, B) @@ -1988,7 +2018,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { case Op_MinHF: case Op_MaxD: case Op_MinD: - return false; + return; // AddINode::Identity @@ -2002,12 +2032,12 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 case Op_AddI: case Op_AddL: - return false; + return; // AbsINode::Identity // Not investigated yet. case Op_AbsI: - return false; + return; } if (n->is_Load()) { @@ -2021,7 +2051,7 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // // Found with: // java -XX:VerifyIterativeGVN=1000 -Xcomp --version - return false; + return; } if (n->is_Store()) { @@ -2032,20 +2062,20 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { // Found with: // applications/ctw/modules/java_base_2.java // -ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation -Djava.awt.headless=true -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 - return false; + return; } if (n->is_Vector()) { // Found with tier1-3. Not investigated yet. // The observed issue was with AndVNode::Identity - return false; + return; } Node* i = n->Identity(this); // If we cannot find any other Identity, we are happy. if (i == n) { verify_empty_worklist(n); - return false; + return; } // The verification just found a new Identity that was not found during IGVN. @@ -2057,11 +2087,12 @@ bool PhaseIterGVN::verify_Identity_for(Node* n) { ss.print_cr("New node:"); i->dump_bfs(1, nullptr, "", &ss); tty->print_cr("%s", ss.as_string()); - return true; + + assert(false, "Missed Identity optimization opportunity in PhaseIterGVN for %s", n->Name()); } // Some other verifications that are not specific to a particular transformation. -bool PhaseIterGVN::verify_node_invariants_for(const Node* n) { +void 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. @@ -2069,10 +2100,10 @@ bool PhaseIterGVN::verify_node_invariants_for(const Node* n) { 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; + + assert(false, "Broken node invariant for %s", n->Name()); } } - return false; } #endif @@ -2729,6 +2760,7 @@ uint PhaseCCP::_total_constants = 0; PhaseCCP::PhaseCCP( PhaseIterGVN *igvn ) : PhaseIterGVN(igvn) { NOT_PRODUCT( clear_constants(); ) assert( _worklist.size() == 0, "" ); + _phase = PhaseValuesType::ccp; analyze(); } @@ -2808,16 +2840,18 @@ void PhaseCCP::analyze() { // For every node n on verify list, check if type(n) == n->Value() // We have a list of exceptions, see comments in verify_Value_for. void PhaseCCP::verify_analyze(Unique_Node_List& worklist_verify) { - bool failure = false; while (worklist_verify.size()) { Node* n = worklist_verify.pop(); - failure |= verify_Value_for(n); + + // An assert in verify_Value_for means that PhaseCCP is not at fixpoint + // and that the analysis result may be unsound. + // If this happens, check why the reported nodes were not processed again in CCP. + // We should either make sure that these nodes are properly added back to the CCP worklist + // in PhaseCCP::push_child_nodes_to_worklist() to update their type in the same round, + // or that they are added in PhaseCCP::needs_revisit() so that analysis revisits + // them at the end of the round. + verify_Value_for(n, true); } - // If we get this assert, check why the reported nodes were not processed again in CCP. - // We should either make sure that these nodes are properly added back to the CCP worklist - // in PhaseCCP::push_child_nodes_to_worklist() to update their type or add an exception - // in the verification code above if that is not possible for some reason (like Load nodes). - assert(!failure, "PhaseCCP not at fixpoint: analysis result may be unsound."); } #endif diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index e436a61fa73..a54c087d8ae 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -224,7 +224,13 @@ class PhaseTransform : public Phase { // 3) NodeHash table, to find identical nodes (and remove/update the hash of a node on modification). class PhaseValues : public PhaseTransform { protected: - bool _iterGVN; + enum class PhaseValuesType { + gvn, + iter_gvn, + ccp + }; + + PhaseValuesType _phase; // Hash table for value-numbering. Reference to "C->node_hash()", NodeHash &_table; @@ -247,7 +253,7 @@ class PhaseValues : public PhaseTransform { void init_con_caches(); public: - PhaseValues() : PhaseTransform(GVN), _iterGVN(false), + PhaseValues() : PhaseTransform(GVN), _phase(PhaseValuesType::gvn), _table(*C->node_hash()), _types(*C->types()) { NOT_PRODUCT( clear_new_values(); ) @@ -256,7 +262,7 @@ class PhaseValues : public PhaseTransform { init_con_caches(); } NOT_PRODUCT(~PhaseValues();) - PhaseIterGVN* is_IterGVN() { return (_iterGVN) ? (PhaseIterGVN*)this : nullptr; } + PhaseIterGVN* is_IterGVN(); // Some Ideal and other transforms delete --> modify --> insert values bool hash_delete(Node* n) { return _table.hash_delete(n); } @@ -494,10 +500,10 @@ class PhaseIterGVN : public PhaseGVN { void optimize(); #ifdef ASSERT void verify_optimize(); - bool verify_Value_for(Node* n); - 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_Value_for(const Node* n, bool strict = false); + void verify_Ideal_for(Node* n, bool can_reshape); + void verify_Identity_for(Node* n); + void verify_node_invariants_for(const Node* n); void verify_empty_worklist(Node* n); #endif From 9e4f0225e9b3e9f21f9cd6007bbae43e0f6e4702 Mon Sep 17 00:00:00 2001 From: Kerem Kat Date: Wed, 1 Apr 2026 00:10:02 +0000 Subject: [PATCH 7/7] 8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN Reviewed-by: qamai, aseoane --- src/hotspot/share/opto/c2_globals.hpp | 4 + src/hotspot/share/opto/cfgnode.cpp | 4 +- src/hotspot/share/opto/compile.cpp | 10 +- src/hotspot/share/opto/escape.cpp | 2 +- src/hotspot/share/opto/graphKit.cpp | 4 +- src/hotspot/share/opto/ifnode.cpp | 29 +- src/hotspot/share/opto/loopnode.cpp | 8 +- src/hotspot/share/opto/loopopts.cpp | 18 +- src/hotspot/share/opto/macro.cpp | 24 +- src/hotspot/share/opto/memnode.cpp | 2 +- src/hotspot/share/opto/phaseX.cpp | 290 +++++++++++------- src/hotspot/share/opto/phaseX.hpp | 78 ++++- src/hotspot/share/opto/split_if.cpp | 24 +- .../compiler/arraycopy/TestCloneAccess.java | 12 +- .../igvn/TestFoldComparesCleanup.java | 56 ++++ .../compiler/lib/ir_framework/IRNode.java | 5 + .../aotClassLinking/TestDeepIGVNRevisit.java | 83 +++++ 17 files changed, 467 insertions(+), 186 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/igvn/TestFoldComparesCleanup.java create mode 100644 test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/TestDeepIGVNRevisit.java diff --git a/src/hotspot/share/opto/c2_globals.hpp b/src/hotspot/share/opto/c2_globals.hpp index 4160ac8555b..b1a51da2e78 100644 --- a/src/hotspot/share/opto/c2_globals.hpp +++ b/src/hotspot/share/opto/c2_globals.hpp @@ -677,6 +677,10 @@ develop(bool, TraceIterativeGVN, false, \ "Print progress during Iterative Global Value Numbering") \ \ + develop(bool, UseDeepIGVNRevisit, true, \ + "Re-process nodes that could benefit from a deep revisit after " \ + "the IGVN worklist drains") \ + \ develop(uint, VerifyIterativeGVN, 0, \ "Verify Iterative Global Value Numbering =EDCBA, with:" \ " E: verify node specific invariants" \ diff --git a/src/hotspot/share/opto/cfgnode.cpp b/src/hotspot/share/opto/cfgnode.cpp index cdade0b7f0b..dab9fa6166c 100644 --- a/src/hotspot/share/opto/cfgnode.cpp +++ b/src/hotspot/share/opto/cfgnode.cpp @@ -735,7 +735,7 @@ Node *RegionNode::Ideal(PhaseGVN *phase, bool can_reshape) { #endif } // Remove the RegionNode itself from DefUse info - igvn->remove_dead_node(this); + igvn->remove_dead_node(this, PhaseIterGVN::NodeOrigin::Graph); return nullptr; } return this; // Record progress @@ -1007,7 +1007,7 @@ bool RegionNode::optimize_trichotomy(PhaseIterGVN* igvn) { BoolNode* new_bol = new BoolNode(bol2->in(1), res); igvn->replace_input_of(iff2, 1, igvn->transform((proj2->_con == 1) ? new_bol : new_bol->negate(igvn))); if (new_bol->outcnt() == 0) { - igvn->remove_dead_node(new_bol); + igvn->remove_dead_node(new_bol, PhaseIterGVN::NodeOrigin::Speculative); } } return false; diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp index 0c341c245b5..511a3577c4b 100644 --- a/src/hotspot/share/opto/compile.cpp +++ b/src/hotspot/share/opto/compile.cpp @@ -2280,7 +2280,7 @@ void Compile::remove_root_to_sfpts_edges(PhaseIterGVN& igvn) { if (n != nullptr && n->is_SafePoint()) { r->rm_prec(i); if (n->outcnt() == 0) { - igvn.remove_dead_node(n); + igvn.remove_dead_node(n, PhaseIterGVN::NodeOrigin::Graph); } --i; } @@ -2325,7 +2325,7 @@ void Compile::Optimize() { #endif { TracePhase tp(_t_iterGVN); - igvn.optimize(); + igvn.optimize(true); } if (failing()) return; @@ -2389,7 +2389,7 @@ void Compile::Optimize() { PhaseRenumberLive prl(initial_gvn(), *igvn_worklist()); } igvn.reset_from_gvn(initial_gvn()); - igvn.optimize(); + igvn.optimize(true); if (failing()) return; } @@ -2421,7 +2421,7 @@ void Compile::Optimize() { int mcount = macro_count(); // Record number of allocations and locks before IGVN // Optimize out fields loads from scalar replaceable allocations. - igvn.optimize(); + igvn.optimize(true); print_method(PHASE_ITER_GVN_AFTER_EA, 2); if (failing()) return; @@ -2500,7 +2500,7 @@ void Compile::Optimize() { { TracePhase tp(_t_iterGVN2); igvn.reset_from_igvn(&ccp); - igvn.optimize(); + igvn.optimize(true); } print_method(PHASE_ITER_GVN2, 2); diff --git a/src/hotspot/share/opto/escape.cpp b/src/hotspot/share/opto/escape.cpp index e33e6fa6e86..11b7ea96552 100644 --- a/src/hotspot/share/opto/escape.cpp +++ b/src/hotspot/share/opto/escape.cpp @@ -918,7 +918,7 @@ void ConnectionGraph::reduce_phi_on_castpp_field_load(Node* curr_castpp, Growabl j = MIN2(j, (int)use->outcnt()-1); } - _igvn->remove_dead_node(use); + _igvn->remove_dead_node(use, PhaseIterGVN::NodeOrigin::Graph); } --i; i = MIN2(i, (int)curr_castpp->outcnt()-1); diff --git a/src/hotspot/share/opto/graphKit.cpp b/src/hotspot/share/opto/graphKit.cpp index fa9ee63b440..368d03f32ee 100644 --- a/src/hotspot/share/opto/graphKit.cpp +++ b/src/hotspot/share/opto/graphKit.cpp @@ -2812,8 +2812,8 @@ Node* Phase::gen_subtype_check(Node* subklass, Node* superklass, Node** ctrl, No *ctrl = iftrue1; // We need exactly the 1 test above PhaseIterGVN* igvn = gvn.is_IterGVN(); if (igvn != nullptr) { - igvn->remove_globally_dead_node(r_ok_subtype); - igvn->remove_globally_dead_node(r_not_subtype); + igvn->remove_globally_dead_node(r_ok_subtype, PhaseIterGVN::NodeOrigin::Speculative); + igvn->remove_globally_dead_node(r_not_subtype, PhaseIterGVN::NodeOrigin::Speculative); } return not_subtype_ctrl; } diff --git a/src/hotspot/share/opto/ifnode.cpp b/src/hotspot/share/opto/ifnode.cpp index 384e5f8673d..0b2cacdb801 100644 --- a/src/hotspot/share/opto/ifnode.cpp +++ b/src/hotspot/share/opto/ifnode.cpp @@ -132,7 +132,7 @@ static Node* split_if(IfNode *iff, PhaseIterGVN *igvn) { cmp2->set_req(2,con2); const Type *t = cmp2->Value(igvn); // This compare is dead, so whack it! - igvn->remove_dead_node(cmp2); + igvn->remove_dead_node(cmp2, PhaseIterGVN::NodeOrigin::Speculative); if( !t->singleton() ) return nullptr; // No intervening control, like a simple Call @@ -443,7 +443,7 @@ static Node* split_if(IfNode *iff, PhaseIterGVN *igvn) { } l -= uses_found; // we deleted 1 or more copies of this edge } - igvn->remove_dead_node(p); + igvn->remove_dead_node(p, PhaseIterGVN::NodeOrigin::Graph); } // Force the original merge dead @@ -455,14 +455,14 @@ static Node* split_if(IfNode *iff, PhaseIterGVN *igvn) { r->set_req(0, nullptr); } else { assert(u->outcnt() == 0, "only dead users"); - igvn->remove_dead_node(u); + igvn->remove_dead_node(u, PhaseIterGVN::NodeOrigin::Graph); } l -= 1; } - igvn->remove_dead_node(r); + igvn->remove_dead_node(r, PhaseIterGVN::NodeOrigin::Graph); // Now remove the bogus extra edges used to keep things alive - igvn->remove_dead_node( hook ); + igvn->remove_dead_node(hook, PhaseIterGVN::NodeOrigin::Speculative); // Must return either the original node (now dead) or a new node // (Do not return a top here, since that would break the uniqueness of top.) @@ -906,6 +906,7 @@ bool IfNode::fold_compares_helper(ProjNode* proj, ProjNode* success, ProjNode* f IfNode* dom_iff = proj->in(0)->as_If(); BoolNode* dom_bool = dom_iff->in(1)->as_Bool(); Node* lo = dom_iff->in(1)->in(1)->in(2); + Node* orig_lo = lo; Node* hi = this_cmp->in(2); Node* n = this_cmp->in(1); ProjNode* otherproj = proj->other_if_proj(); @@ -917,6 +918,7 @@ bool IfNode::fold_compares_helper(ProjNode* proj, ProjNode* success, ProjNode* f BoolTest::mask hi_test = this_bool->_test._test; BoolTest::mask cond = hi_test; + PhaseTransform::SpeculativeProgressGuard progress_guard(igvn); // convert: // // dom_bool = x {<,<=,>,>=} a @@ -1055,6 +1057,7 @@ bool IfNode::fold_compares_helper(ProjNode* proj, ProjNode* success, ProjNode* f // previous if determines the result of this if so // replace Bool with constant igvn->replace_input_of(this, 1, igvn->intcon(success->_con)); + progress_guard.commit(); return true; } } @@ -1089,11 +1092,14 @@ bool IfNode::fold_compares_helper(ProjNode* proj, ProjNode* success, ProjNode* f // min(limit, max(-2 + min_jint + 1, min_jint)) // = min(limit, min_jint) // = min_jint + if (lo != orig_lo && lo->outcnt() == 0) { + igvn->remove_dead_node(lo, PhaseIterGVN::NodeOrigin::Speculative); + } if (adjusted_val->outcnt() == 0) { - igvn->remove_dead_node(adjusted_val); + igvn->remove_dead_node(adjusted_val, PhaseIterGVN::NodeOrigin::Speculative); } if (adjusted_lim->outcnt() == 0) { - igvn->remove_dead_node(adjusted_lim); + igvn->remove_dead_node(adjusted_lim, PhaseIterGVN::NodeOrigin::Speculative); } igvn->C->record_for_post_loop_opts_igvn(this); return false; @@ -1105,6 +1111,7 @@ bool IfNode::fold_compares_helper(ProjNode* proj, ProjNode* success, ProjNode* f igvn->replace_input_of(dom_iff, 1, igvn->intcon(proj->_con)); igvn->replace_input_of(this, 1, newbool); + progress_guard.commit(); return true; } @@ -1598,11 +1605,11 @@ Node* IfNode::dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool pin_array_ac } } // End for each child of a projection - igvn->remove_dead_node(ifp); + igvn->remove_dead_node(ifp, PhaseIterGVN::NodeOrigin::Graph); } // End for each IfTrue/IfFalse child of If // Kill the IfNode - igvn->remove_dead_node(this); + igvn->remove_dead_node(this, PhaseIterGVN::NodeOrigin::Graph); // Must return either the original node (now dead) or a new node // (Do not return a top here, since that would break the uniqueness of top.) @@ -1815,7 +1822,7 @@ Node* IfNode::simple_subsuming(PhaseIterGVN* igvn) { } if (bol->outcnt() == 0) { - igvn->remove_dead_node(bol); // Kill the BoolNode. + igvn->remove_dead_node(bol, PhaseIterGVN::NodeOrigin::Graph); // Kill the BoolNode. } return this; } @@ -1961,7 +1968,7 @@ static IfNode* idealize_test(PhaseGVN* phase, IfNode* iff) { Node *prior = igvn->hash_find_insert(iff); if( prior ) { - igvn->remove_dead_node(iff); + igvn->remove_dead_node(iff, PhaseIterGVN::NodeOrigin::Graph); iff = (IfNode*)prior; } else { // Cannot call transform on it just yet diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index 759b22041c1..4a2becc7d59 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/src/hotspot/share/opto/loopnode.cpp @@ -754,7 +754,7 @@ SafePointNode* PhaseIdealLoop::find_safepoint(Node* back_control, Node* x, Ideal } #ifdef ASSERT if (mm != nullptr) { - _igvn.remove_dead_node(mm); + _igvn.remove_dead_node(mm, PhaseIterGVN::NodeOrigin::Speculative); } #endif } @@ -1587,7 +1587,7 @@ bool PhaseIdealLoop::convert_to_long_loop(Node* cmp, Node* phi, IdealLoopTree* l Node* n = iv_nodes.at(i); Node* clone = old_new[n->_idx]; if (clone != nullptr) { - _igvn.remove_dead_node(clone); + _igvn.remove_dead_node(clone, PhaseIterGVN::NodeOrigin::Speculative); } } return false; @@ -4222,7 +4222,7 @@ void PhaseIdealLoop::replace_parallel_iv(IdealLoopTree *loop) { _igvn.replace_node( phi2, add ); // Sometimes an induction variable is unused if (add->outcnt() == 0) { - _igvn.remove_dead_node(add); + _igvn.remove_dead_node(add, PhaseIterGVN::NodeOrigin::Graph); } --i; // deleted this phi; rescan starting with next position } @@ -4875,7 +4875,7 @@ void PhaseIdealLoop::build_and_optimize() { // clear out the dead code after build_loop_late while (_deadlist.size()) { - _igvn.remove_globally_dead_node(_deadlist.pop()); + _igvn.remove_globally_dead_node(_deadlist.pop(), PhaseIterGVN::NodeOrigin::Graph); } eliminate_useless_zero_trip_guard(); diff --git a/src/hotspot/share/opto/loopopts.cpp b/src/hotspot/share/opto/loopopts.cpp index 23902d9c5ca..ba7b37c6841 100644 --- a/src/hotspot/share/opto/loopopts.cpp +++ b/src/hotspot/share/opto/loopopts.cpp @@ -161,7 +161,7 @@ Node* PhaseIdealLoop::split_thru_phi(Node* n, Node* region, int policy) { } if (the_clone != x) { - _igvn.remove_dead_node(the_clone); + _igvn.remove_dead_node(the_clone, PhaseIterGVN::NodeOrigin::Speculative); } else if (region->is_Loop() && i == LoopNode::LoopBackControl && n->is_Load() && can_move_to_inner_loop(n, region->as_Loop(), x)) { // it is not a win if 'x' moved from an outer to an inner loop @@ -172,7 +172,7 @@ Node* PhaseIdealLoop::split_thru_phi(Node* n, Node* region, int policy) { } // Too few wins? if (wins <= policy) { - _igvn.remove_dead_node(phi); + _igvn.remove_dead_node(phi, PhaseIterGVN::NodeOrigin::Speculative); return nullptr; } @@ -1833,7 +1833,7 @@ void PhaseIdealLoop::try_sink_out_of_loop(Node* n) { assert(cast != nullptr, "must have added a cast to pin the node"); } } - _igvn.remove_dead_node(n); + _igvn.remove_dead_node(n, PhaseIterGVN::NodeOrigin::Graph); } _dom_lca_tags_round = 0; } @@ -2040,7 +2040,7 @@ Node* PhaseIdealLoop::clone_iff(PhiNode* phi) { // Register with optimizer Node *hit1 = _igvn.hash_find_insert(phi1); if (hit1) { // Hit, toss just made Phi - _igvn.remove_dead_node(phi1); // Remove new phi + _igvn.remove_dead_node(phi1, PhaseIterGVN::NodeOrigin::Speculative); // Remove new phi assert(hit1->is_Phi(), "" ); phi1 = (PhiNode*)hit1; // Use existing phi } else { // Miss @@ -2048,7 +2048,7 @@ Node* PhaseIdealLoop::clone_iff(PhiNode* phi) { } Node *hit2 = _igvn.hash_find_insert(phi2); if (hit2) { // Hit, toss just made Phi - _igvn.remove_dead_node(phi2); // Remove new phi + _igvn.remove_dead_node(phi2, PhaseIterGVN::NodeOrigin::Speculative); // Remove new phi assert(hit2->is_Phi(), "" ); phi2 = (PhiNode*)hit2; // Use existing phi } else { // Miss @@ -2123,7 +2123,7 @@ CmpNode*PhaseIdealLoop::clone_bool(PhiNode* phi) { // Register with optimizer Node *hit1 = _igvn.hash_find_insert(phi1); if( hit1 ) { // Hit, toss just made Phi - _igvn.remove_dead_node(phi1); // Remove new phi + _igvn.remove_dead_node(phi1, PhaseIterGVN::NodeOrigin::Speculative); // Remove new phi assert( hit1->is_Phi(), "" ); phi1 = (PhiNode*)hit1; // Use existing phi } else { // Miss @@ -2131,7 +2131,7 @@ CmpNode*PhaseIdealLoop::clone_bool(PhiNode* phi) { } Node *hit2 = _igvn.hash_find_insert(phi2); if( hit2 ) { // Hit, toss just made Phi - _igvn.remove_dead_node(phi2); // Remove new phi + _igvn.remove_dead_node(phi2, PhaseIterGVN::NodeOrigin::Speculative); // Remove new phi assert( hit2->is_Phi(), "" ); phi2 = (PhiNode*)hit2; // Use existing phi } else { // Miss @@ -2282,7 +2282,7 @@ void PhaseIdealLoop::clone_loop_handle_data_uses(Node* old, Node_List &old_new, _igvn.register_new_node_with_optimizer(phi); // Register new phi } else { // or // Remove the new phi from the graph and use the hit - _igvn.remove_dead_node(phi); + _igvn.remove_dead_node(phi, phi == prev ? PhaseIterGVN::NodeOrigin::Graph : PhaseIterGVN::NodeOrigin::Speculative); phi = hit; // Use existing phi } set_ctrl(phi, prev); @@ -3435,7 +3435,7 @@ void PhaseIdealLoop::insert_phi_for_loop( Node* use, uint idx, Node* lp_entry_va set_ctrl(phi, lp); } else { // Remove the new phi from the graph and use the hit - _igvn.remove_dead_node(phi); + _igvn.remove_dead_node(phi, PhaseIterGVN::NodeOrigin::Speculative); phi = hit; } _igvn.replace_input_of(use, idx, phi); diff --git a/src/hotspot/share/opto/macro.cpp b/src/hotspot/share/opto/macro.cpp index f0845ff12ba..300594d5891 100644 --- a/src/hotspot/share/opto/macro.cpp +++ b/src/hotspot/share/opto/macro.cpp @@ -979,7 +979,7 @@ void PhaseMacroExpand::process_users_of_allocation(CallNode *alloc) { } k -= (oc2 - use->outcnt()); } - _igvn.remove_dead_node(use); + _igvn.remove_dead_node(use, PhaseIterGVN::NodeOrigin::Graph); } else if (use->is_ArrayCopy()) { // Disconnect ArrayCopy node ArrayCopyNode* ac = use->as_ArrayCopy(); @@ -1014,7 +1014,7 @@ void PhaseMacroExpand::process_users_of_allocation(CallNode *alloc) { // src can be top at this point if src and dest of the // arraycopy were the same if (src->outcnt() == 0 && !src->is_top()) { - _igvn.remove_dead_node(src); + _igvn.remove_dead_node(src, PhaseIterGVN::NodeOrigin::Graph); } } _igvn._worklist.push(ac); @@ -1024,7 +1024,7 @@ void PhaseMacroExpand::process_users_of_allocation(CallNode *alloc) { j -= (oc1 - res->outcnt()); } assert(res->outcnt() == 0, "all uses of allocated objects must be deleted"); - _igvn.remove_dead_node(res); + _igvn.remove_dead_node(res, PhaseIterGVN::NodeOrigin::Graph); } // @@ -1506,7 +1506,7 @@ void PhaseMacroExpand::expand_allocate_common( transform_later(_callprojs.fallthrough_memproj); } migrate_outs(_callprojs.catchall_memproj, _callprojs.fallthrough_memproj); - _igvn.remove_dead_node(_callprojs.catchall_memproj); + _igvn.remove_dead_node(_callprojs.catchall_memproj, PhaseIterGVN::NodeOrigin::Graph); } // An allocate node has separate i_o projections for the uses on the control @@ -1525,7 +1525,7 @@ void PhaseMacroExpand::expand_allocate_common( transform_later(_callprojs.fallthrough_ioproj); } migrate_outs(_callprojs.catchall_ioproj, _callprojs.fallthrough_ioproj); - _igvn.remove_dead_node(_callprojs.catchall_ioproj); + _igvn.remove_dead_node(_callprojs.catchall_ioproj, PhaseIterGVN::NodeOrigin::Graph); } // if we generated only a slow call, we are done @@ -1589,11 +1589,11 @@ void PhaseMacroExpand::yank_alloc_node(AllocateNode* alloc) { --i; // back up iterator } assert(_callprojs.resproj->outcnt() == 0, "all uses must be deleted"); - _igvn.remove_dead_node(_callprojs.resproj); + _igvn.remove_dead_node(_callprojs.resproj, PhaseIterGVN::NodeOrigin::Graph); } if (_callprojs.fallthrough_catchproj != nullptr) { migrate_outs(_callprojs.fallthrough_catchproj, ctrl); - _igvn.remove_dead_node(_callprojs.fallthrough_catchproj); + _igvn.remove_dead_node(_callprojs.fallthrough_catchproj, PhaseIterGVN::NodeOrigin::Graph); } if (_callprojs.catchall_catchproj != nullptr) { _igvn.rehash_node_delayed(_callprojs.catchall_catchproj); @@ -1601,16 +1601,16 @@ void PhaseMacroExpand::yank_alloc_node(AllocateNode* alloc) { } if (_callprojs.fallthrough_proj != nullptr) { Node* catchnode = _callprojs.fallthrough_proj->unique_ctrl_out(); - _igvn.remove_dead_node(catchnode); - _igvn.remove_dead_node(_callprojs.fallthrough_proj); + _igvn.remove_dead_node(catchnode, PhaseIterGVN::NodeOrigin::Graph); + _igvn.remove_dead_node(_callprojs.fallthrough_proj, PhaseIterGVN::NodeOrigin::Graph); } if (_callprojs.fallthrough_memproj != nullptr) { migrate_outs(_callprojs.fallthrough_memproj, mem); - _igvn.remove_dead_node(_callprojs.fallthrough_memproj); + _igvn.remove_dead_node(_callprojs.fallthrough_memproj, PhaseIterGVN::NodeOrigin::Graph); } if (_callprojs.fallthrough_ioproj != nullptr) { migrate_outs(_callprojs.fallthrough_ioproj, i_o); - _igvn.remove_dead_node(_callprojs.fallthrough_ioproj); + _igvn.remove_dead_node(_callprojs.fallthrough_ioproj, PhaseIterGVN::NodeOrigin::Graph); } if (_callprojs.catchall_memproj != nullptr) { _igvn.rehash_node_delayed(_callprojs.catchall_memproj); @@ -1629,7 +1629,7 @@ void PhaseMacroExpand::yank_alloc_node(AllocateNode* alloc) { } } #endif - _igvn.remove_dead_node(alloc); + _igvn.remove_dead_node(alloc, PhaseIterGVN::NodeOrigin::Graph); } void PhaseMacroExpand::expand_initialize_membar(AllocateNode* alloc, InitializeNode* init, diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp index 8f38da7de75..2d93dd6d22e 100644 --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -1812,7 +1812,7 @@ Node* LoadNode::split_through_phi(PhaseGVN* phase, bool ignore_missing_instance_ } } if (x != the_clone && the_clone != nullptr) { - igvn->remove_dead_node(the_clone); + igvn->remove_dead_node(the_clone, PhaseIterGVN::NodeOrigin::Speculative); } phi->set_req(i, x); } diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index d82bc34b847..762ca030fb7 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -564,7 +564,7 @@ PhaseValues::~PhaseValues() { _table.dump(); // Statistics for value progress and efficiency if( PrintCompilation && Verbose && WizardMode ) { - tty->print("\n%sValues: %d nodes ---> %d/%d (%d)", + tty->print("\n%sValues: %d nodes ---> " UINT64_FORMAT "/%d (%d)", is_IterGVN() ? "Iter" : " ", C->unique(), made_progress(), made_transforms(), made_new_values()); if( made_transforms() != 0 ) { tty->print_cr(" ratio %f", made_progress()/(float)made_transforms() ); @@ -720,14 +720,14 @@ Node* PhaseGVN::transform(Node* n) { } if (t->singleton() && !k->is_Con()) { - NOT_PRODUCT(set_progress();) + set_progress(); return makecon(t); // Turn into a constant } // Now check for Identities i = k->Identity(this); // Look for a nearby replacement if (i != k) { // Found? Return replacement! - NOT_PRODUCT(set_progress();) + set_progress(); return i; } @@ -735,7 +735,7 @@ Node* PhaseGVN::transform(Node* n) { i = hash_find_insert(k); // Insert if new if (i && (i != k)) { // Return the pre-existing node - NOT_PRODUCT(set_progress();) + set_progress(); return i; } @@ -956,7 +956,7 @@ void PhaseIterGVN::init_verifyPhaseIterGVN() { #endif } -void PhaseIterGVN::verify_PhaseIterGVN() { +void PhaseIterGVN::verify_PhaseIterGVN(bool deep_revisit_converged) { #ifdef ASSERT // Verify nodes with changed inputs. Unique_Node_List* modified_list = C->modified_nodes(); @@ -989,7 +989,7 @@ void PhaseIterGVN::verify_PhaseIterGVN() { } } - verify_optimize(); + verify_optimize(deep_revisit_converged); #endif } #endif /* PRODUCT */ @@ -1019,38 +1019,54 @@ void PhaseIterGVN::trace_PhaseIterGVN_verbose(Node* n, int num_processed) { } #endif /* ASSERT */ -void PhaseIterGVN::optimize() { - DEBUG_ONLY(uint num_processed = 0;) - NOT_PRODUCT(init_verifyPhaseIterGVN();) - NOT_PRODUCT(C->reset_igv_phase_iter(PHASE_AFTER_ITER_GVN_STEP);) - C->print_method(PHASE_BEFORE_ITER_GVN, 3); - if (StressIGVN) { - shuffle_worklist(); +bool PhaseIterGVN::needs_deep_revisit(const Node* n) const { + // LoadNode::Value() -> can_see_stored_value() walks up through many memory + // nodes. LoadNode::Ideal() -> find_previous_store() also walks up to 50 + // nodes through stores and arraycopy nodes. + if (n->is_Load()) { + return true; + } + // CmpPNode::sub() -> detect_ptr_independence() -> all_controls_dominate() + // walks CFG dominator relationships extensively. This only triggers when + // both inputs are oop pointers (subnode.cpp:984). + if (n->Opcode() == Op_CmpP) { + const Type* t1 = type_or_null(n->in(1)); + const Type* t2 = type_or_null(n->in(2)); + return t1 != nullptr && t1->isa_oopptr() && + t2 != nullptr && t2->isa_oopptr(); + } + // IfNode::Ideal() -> search_identical() walks up the CFG dominator tree. + // RangeCheckNode::Ideal() scans up to ~999 nodes up the chain. + // CountedLoopEndNode/LongCountedLoopEndNode::Ideal() via simple_subsuming + // looks for dominating test that subsumes the current test. + switch (n->Opcode()) { + case Op_If: + case Op_RangeCheck: + case Op_CountedLoopEnd: + case Op_LongCountedLoopEnd: + return true; + default: + break; } + return false; +} - // The node count check in the loop below (check_node_count) assumes that we - // increase the live node count with at most - // max_live_nodes_increase_per_iteration in between checks. If this - // assumption does not hold, there is a risk that we exceed the max node - // limit in between checks and trigger an assert during node creation. +bool PhaseIterGVN::drain_worklist() { + uint loop_count = 1; const int max_live_nodes_increase_per_iteration = NodeLimitFudgeFactor * 3; - - uint loop_count = 0; - // Pull from worklist and transform the node. If the node has changed, - // update edge info and put uses on worklist. - while (_worklist.size() > 0) { + while (_worklist.size() != 0) { if (C->check_node_count(max_live_nodes_increase_per_iteration, "Out of nodes")) { C->print_method(PHASE_AFTER_ITER_GVN, 3); - return; + return true; } Node* n = _worklist.pop(); if (loop_count >= K * C->live_nodes()) { - DEBUG_ONLY(dump_infinite_loop_info(n, "PhaseIterGVN::optimize");) - C->record_method_not_compilable("infinite loop in PhaseIterGVN::optimize"); + DEBUG_ONLY(dump_infinite_loop_info(n, "PhaseIterGVN::drain_worklist");) + C->record_method_not_compilable("infinite loop in PhaseIterGVN::drain_worklist"); C->print_method(PHASE_AFTER_ITER_GVN, 3); - return; + return true; } - DEBUG_ONLY(trace_PhaseIterGVN_verbose(n, num_processed++);) + DEBUG_ONLY(trace_PhaseIterGVN_verbose(n, _num_processed++);) if (n->outcnt() != 0) { NOT_PRODUCT(const Type* oldtype = type_or_null(n)); // Do the transformation @@ -1058,7 +1074,7 @@ void PhaseIterGVN::optimize() { Node* nn = transform_old(n); DEBUG_ONLY(int live_nodes_after = C->live_nodes();) // Ensure we did not increase the live node count with more than - // max_live_nodes_increase_per_iteration during the call to transform_old + // max_live_nodes_increase_per_iteration during the call to transform_old. DEBUG_ONLY(int increase = live_nodes_after - live_nodes_before;) assert(increase < max_live_nodes_increase_per_iteration, "excessive live node increase in single iteration of IGVN: %d " @@ -1066,16 +1082,115 @@ void PhaseIterGVN::optimize() { increase, max_live_nodes_increase_per_iteration); NOT_PRODUCT(trace_PhaseIterGVN(n, nn, oldtype);) } else if (!n->is_top()) { - remove_dead_node(n); + remove_dead_node(n, NodeOrigin::Graph); } loop_count++; } - NOT_PRODUCT(verify_PhaseIterGVN();) + return false; +} + +void PhaseIterGVN::push_deep_revisit_candidates() { + ResourceMark rm; + Unique_Node_List all_nodes; + all_nodes.push(C->root()); + for (uint j = 0; j < all_nodes.size(); j++) { + Node* n = all_nodes.at(j); + if (needs_deep_revisit(n)) { + _worklist.push(n); + } + for (DUIterator_Fast imax, i = n->fast_outs(imax); i < imax; i++) { + all_nodes.push(n->fast_out(i)); + } + } +} + +bool PhaseIterGVN::deep_revisit() { + // Re-process nodes that inspect the graph deeply. After the main worklist drains, walk + // the graph to find all live deep-inspection nodes and push them to the worklist + // for re-evaluation. If any produce changes, drain the worklist again. + // Repeat until stable. This mirrors PhaseCCP::analyze()'s revisit loop. + const uint max_deep_revisit_rounds = 10; // typically converges in <2 rounds + uint round = 0; + for (; round < max_deep_revisit_rounds; round++) { + push_deep_revisit_candidates(); + if (_worklist.size() == 0) { + break; // No deep-inspection nodes to revisit, done. + } + +#ifndef PRODUCT + uint candidates = _worklist.size(); + uint n_if = 0; uint n_rc = 0; uint n_load = 0; uint n_cmpp = 0; uint n_cle = 0; uint n_lcle = 0; + if (TraceIterativeGVN) { + for (uint i = 0; i < _worklist.size(); i++) { + Node* n = _worklist.at(i); + switch (n->Opcode()) { + case Op_If: n_if++; break; + case Op_RangeCheck: n_rc++; break; + case Op_CountedLoopEnd: n_cle++; break; + case Op_LongCountedLoopEnd: n_lcle++; break; + case Op_CmpP: n_cmpp++; break; + default: if (n->is_Load()) n_load++; break; + } + } + } +#endif + + // Convergence: if the drain does not make progress (no Ideal, Value, Identity or GVN changes), + // we are at a fixed point. We use made_progress() rather than live_nodes because live_nodes + // misses non-structural changes like a LoadNode dropping its control input. + uint progress_before = made_progress(); + if (drain_worklist()) { + return false; + } + uint progress = made_progress() - progress_before; + +#ifndef PRODUCT + if (TraceIterativeGVN) { + tty->print("deep_revisit round %u: %u candidates (If=%u RC=%u Load=%u CmpP=%u CLE=%u LCLE=%u), progress=%u (%s)", + round, candidates, n_if, n_rc, n_load, n_cmpp, n_cle, n_lcle, progress, progress != 0 ? "changed" : "converged"); + if (C->method() != nullptr) { + tty->print(", "); + C->method()->print_short_name(tty); + } + tty->cr(); + } +#endif + + if (progress == 0) { + break; + } + } + return round < max_deep_revisit_rounds; +} + +void PhaseIterGVN::optimize(bool deep) { + bool deep_revisit_converged = false; + DEBUG_ONLY(_num_processed = 0;) + NOT_PRODUCT(init_verifyPhaseIterGVN();) + NOT_PRODUCT(C->reset_igv_phase_iter(PHASE_AFTER_ITER_GVN_STEP);) + C->print_method(PHASE_BEFORE_ITER_GVN, 3); + if (StressIGVN) { + shuffle_worklist(); + } + + // Pull from worklist and transform the node. + if (drain_worklist()) { + return; + } + + if (deep && UseDeepIGVNRevisit) { + deep_revisit_converged = deep_revisit(); + if (C->failing()) { + return; + } + } + + NOT_PRODUCT(verify_PhaseIterGVN(deep_revisit_converged);) C->print_method(PHASE_AFTER_ITER_GVN, 3); } #ifdef ASSERT -void PhaseIterGVN::verify_optimize() { +void PhaseIterGVN::verify_optimize(bool deep_revisit_converged) { assert(_worklist.size() == 0, "igvn worklist must be empty before verify"); if (is_verify_Value() || @@ -1093,11 +1208,11 @@ void PhaseIterGVN::verify_optimize() { // in PhaseIterGVN::add_users_to_worklist to update it again or add an exception // in the verification methods below if that is not possible for some reason (like Load nodes). if (is_verify_Value()) { - verify_Value_for(n); + verify_Value_for(n, deep_revisit_converged /* strict */); } if (is_verify_Ideal()) { - verify_Ideal_for(n, false); - verify_Ideal_for(n, true); + verify_Ideal_for(n, false /* can_reshape */, deep_revisit_converged); + verify_Ideal_for(n, true /* can_reshape */, deep_revisit_converged); } if (is_verify_Identity()) { verify_Identity_for(n); @@ -1219,52 +1334,15 @@ void PhaseIterGVN::verify_Value_for(const Node* n, bool strict) { // Check that all Ideal optimizations that could be done were done. // Asserts if it found missed optimization opportunities or encountered unexpected changes, and // returns normally otherwise (no missed optimization, or skipped verification). -void PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { +void PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape, bool deep_revisit_converged) { + if (!deep_revisit_converged && needs_deep_revisit(n)) { + return; + } + // First, we check a list of exceptions, where we skip verification, // because there are known cases where Ideal can optimize after IGVN. // Some may be expected and cannot be fixed, and others should be fixed. switch (n->Opcode()) { - // RangeCheckNode::Ideal looks up the chain for about 999 nodes - // (see "Range-Check scan limit"). So, it is possible that something - // is optimized in that input subgraph, and the RangeCheck was not - // added to the worklist because it would be too expensive to walk - // down the graph for 1000 nodes and put all on the worklist. - // - // Found with: - // java -XX:VerifyIterativeGVN=0100 -Xbatch --version - case Op_RangeCheck: - return; - - // IfNode::Ideal does: - // Node* prev_dom = search_identical(dist, igvn); - // which means we seach up the CFG, traversing at most up to a distance. - // If anything happens rather far away from the If, we may not put the If - // back on the worklist. - // - // Found with: - // java -XX:VerifyIterativeGVN=0100 -Xcomp --version - case Op_If: - return; - - // IfNode::simple_subsuming - // Looks for dominating test that subsumes the current test. - // Notification could be difficult because of larger distance. - // - // Found with: - // runtime/exceptionMsgs/ArrayIndexOutOfBoundsException/ArrayIndexOutOfBoundsExceptionTest.java#id1 - // -XX:VerifyIterativeGVN=1110 - case Op_CountedLoopEnd: - return; - - // LongCountedLoopEndNode::Ideal - // Probably same issue as above. - // - // Found with: - // compiler/predicates/assertion/TestAssertionPredicates.java#NoLoopPredicationXbatch - // -XX:StressLongCountedLoop=2000000 -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 - case Op_LongCountedLoopEnd: - return; - // RegionNode::Ideal does "Skip around the useless IF diamond". // 245 IfTrue === 244 // 258 If === 245 257 @@ -1740,22 +1818,6 @@ void PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { return; } - if (n->is_Load()) { - // LoadNode::Ideal uses tries to find an earlier memory state, and - // checks can_see_stored_value for it. - // - // Investigate why this was not already done during IGVN. - // A similar issue happens with Identity. - // - // There seem to be other cases where loads go up some steps, like - // LoadNode::Ideal going up 10x steps to find dominating load. - // - // Found with: - // test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java - // -XX:VerifyIterativeGVN=1110 - return; - } - if (n->is_Store()) { // StoreNode::Ideal can do this: // // Capture an unaliased, unconditional, simple store into an initializer. @@ -1840,8 +1902,16 @@ void PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { return; } - // The number of nodes shoud not increase. - uint old_unique = C->unique(); + // Ideal should not make progress if it returns nullptr. + // We use made_progress() rather than unique() or live_nodes() because some + // Ideal implementations speculatively create nodes and kill them before + // returning nullptr (e.g. split_if clones a Cmp to check is_canonical). + // unique() is a high-water mark that is not decremented by remove_dead_node, + // so it would cause false-positives. live_nodes() accounts for dead nodes but can + // decrease when Ideal removes existing nodes as side effects. + // made_progress() precisely tracks meaningful transforms, and speculative + // work killed via NodeOrigin::Speculative does not increment it. + uint old_progress = made_progress(); // The hash of a node should not change, this would indicate different inputs uint old_hash = n->hash(); // Remove 'n' from hash table in case it gets modified. We want to avoid @@ -1853,14 +1923,15 @@ void PhaseIterGVN::verify_Ideal_for(Node* n, bool can_reshape) { Node* i = n->Ideal(this, can_reshape); // If there was no new Idealization, we are probably happy. if (i == nullptr) { - if (old_unique < C->unique()) { + uint progress = made_progress() - old_progress; + if (progress != 0) { stringStream ss; // Print as a block without tty lock. ss.cr(); - ss.print_cr("Ideal optimization did not make progress but created new unused nodes."); - ss.print_cr(" old_unique = %d, unique = %d", old_unique, C->unique()); + ss.print_cr("Ideal optimization did not make progress but had side effects."); + ss.print_cr(" %u transforms made progress", progress); n->dump_bfs(1, nullptr, "", &ss); tty->print_cr("%s", ss.as_string()); - assert(false, "Unexpected new unused nodes from applying Ideal optimization on %s", n->Name()); + assert(false, "Unexpected side effects from applying Ideal optimization on %s", n->Name()); } if (old_hash != n->hash()) { @@ -2163,6 +2234,9 @@ Node *PhaseIterGVN::transform_old(Node* n) { #endif DEBUG_ONLY(uint loop_count = 1;) + if (i != nullptr) { + set_progress(); + } while (i != nullptr) { #ifdef ASSERT if (loop_count >= K + C->live_nodes()) { @@ -2202,10 +2276,8 @@ Node *PhaseIterGVN::transform_old(Node* n) { // cache Value. Later requests for the local phase->type of this Node can // use the cached Value instead of suffering with 'bottom_type'. if (type_or_null(k) != t) { -#ifndef PRODUCT - inc_new_values(); + NOT_PRODUCT(inc_new_values();) set_progress(); -#endif set_type(k, t); // If k is a TypeNode, capture any more-precise type permanently into Node k->raise_bottom_type(t); @@ -2214,7 +2286,7 @@ Node *PhaseIterGVN::transform_old(Node* n) { } // If 'k' computes a constant, replace it with a constant if (t->singleton() && !k->is_Con()) { - NOT_PRODUCT(set_progress();) + set_progress(); Node* con = makecon(t); // Make a constant add_users_to_worklist(k); subsume_node(k, con); // Everybody using k now uses con @@ -2224,7 +2296,7 @@ Node *PhaseIterGVN::transform_old(Node* n) { // Now check for Identities i = k->Identity(this); // Look for a nearby replacement if (i != k) { // Found? Return replacement! - NOT_PRODUCT(set_progress();) + set_progress(); add_users_to_worklist(k); subsume_node(k, i); // Everybody using k now uses i return i; @@ -2234,7 +2306,7 @@ Node *PhaseIterGVN::transform_old(Node* n) { i = hash_find_insert(k); // Check for pre-existing node if (i && (i != k)) { // Return the pre-existing node if it isn't dead - NOT_PRODUCT(set_progress();) + set_progress(); add_users_to_worklist(k); subsume_node(k, i); // Everybody using k now uses i return i; @@ -2253,7 +2325,7 @@ const Type* PhaseIterGVN::saturate(const Type* new_type, const Type* old_type, //------------------------------remove_globally_dead_node---------------------- // Kill a globally dead Node. All uses are also globally dead and are // aggressively trimmed. -void PhaseIterGVN::remove_globally_dead_node( Node *dead ) { +void PhaseIterGVN::remove_globally_dead_node(Node* dead, NodeOrigin origin) { enum DeleteProgress { PROCESS_INPUTS, PROCESS_OUTPUTS @@ -2270,11 +2342,13 @@ void PhaseIterGVN::remove_globally_dead_node( Node *dead ) { uint progress_state = stack.index(); assert(dead != C->root(), "killing root, eh?"); assert(!dead->is_top(), "add check for top when pushing"); - NOT_PRODUCT( set_progress(); ) if (progress_state == PROCESS_INPUTS) { // After following inputs, continue to outputs stack.set_index(PROCESS_OUTPUTS); if (!dead->is_Con()) { // Don't kill cons but uses + if (origin != NodeOrigin::Speculative) { + set_progress(); + } bool recurse = false; // Remove from hash table _table.hash_delete( dead ); @@ -2389,7 +2463,7 @@ void PhaseIterGVN::subsume_node( Node *old, Node *nn ) { // Smash all inputs to 'old', isolating him completely Node *temp = new Node(1); temp->init_req(0,nn); // Add a use to nn to prevent him from dying - remove_dead_node( old ); + remove_dead_node(old, NodeOrigin::Graph); temp->del_req(0); // Yank bogus edge if (nn != nullptr && nn->outcnt() == 0) { _worklist.push(nn); diff --git a/src/hotspot/share/opto/phaseX.hpp b/src/hotspot/share/opto/phaseX.hpp index a54c087d8ae..cd6e3efda9a 100644 --- a/src/hotspot/share/opto/phaseX.hpp +++ b/src/hotspot/share/opto/phaseX.hpp @@ -187,8 +187,8 @@ class PhaseRenumberLive : public PhaseRemoveUseless { class PhaseTransform : public Phase { public: PhaseTransform(PhaseNumber pnum) : Phase(pnum) { -#ifndef PRODUCT clear_progress(); +#ifndef PRODUCT clear_transforms(); set_allow_progress(true); #endif @@ -201,12 +201,31 @@ class PhaseTransform : public Phase { // true if CFG node d dominates CFG node n virtual bool is_dominator(Node *d, Node *n) { fatal("unimplemented for this pass"); return false; }; -#ifndef PRODUCT - uint _count_progress; // For profiling, count transforms that make progress - void set_progress() { ++_count_progress; assert( allow_progress(),"No progress allowed during verification"); } - void clear_progress() { _count_progress = 0; } - uint made_progress() const { return _count_progress; } + uint64_t _count_progress; // Count transforms that make progress + void set_progress() { ++_count_progress; assert(allow_progress(), "No progress allowed during verification"); } + void clear_progress() { _count_progress = 0; } + uint64_t made_progress() const { return _count_progress; } + + // RAII guard for speculative transforms. Restores _count_progress in the destructor + // unless commit() is called, so that abandoned speculative work does not count as progress. + // In case multiple nodes are created and only some are speculative, commit() should still be called. + class SpeculativeProgressGuard { + PhaseTransform* _phase; + uint64_t _saved_progress; + bool _committed; + public: + SpeculativeProgressGuard(PhaseTransform* phase) : + _phase(phase), _saved_progress(phase->made_progress()), _committed(false) {} + ~SpeculativeProgressGuard() { + if (!_committed) { + _phase->_count_progress = _saved_progress; + } + } + void commit() { _committed = true; } + }; + +#ifndef PRODUCT uint _count_transforms; // For profiling, count transforms performed void set_transforms() { ++_count_transforms; } void clear_transforms() { _count_transforms = 0; } @@ -446,10 +465,30 @@ class PhaseIterGVN : public PhaseGVN { private: bool _delay_transform; // When true simply register the node when calling transform // instead of actually optimizing it + DEBUG_ONLY(uint _num_processed;) // Running count for trace_PhaseIterGVN_verbose // Idealize old Node 'n' with respect to its inputs and its value virtual Node *transform_old( Node *a_node ); + // Drain the IGVN worklist: process nodes until the worklist is empty. + // Returns true if compilation was aborted (node limit or infinite loop), + // false on normal completion. + bool drain_worklist(); + + // Walk all live nodes and push deep-inspection candidates to _worklist. + void push_deep_revisit_candidates(); + + // After the main worklist drains, re-process deep-inspection nodes to + // catch optimization opportunities from far-away changes. Repeats until + // convergence (no progress made) or max rounds reached. + // Returns true if converged. + bool deep_revisit(); + + // Returns true for nodes that inspect the graph beyond their direct + // inputs, and therefore may miss optimization opportunities when + // changes happen far away. + bool needs_deep_revisit(const Node* n) const; + // Subsume users of node 'old' into node 'nn' void subsume_node( Node *old, Node *nn ); @@ -497,11 +536,16 @@ class PhaseIterGVN : public PhaseGVN { // Given def-use info and an initial worklist, apply Node::Ideal, // Node::Value, Node::Identity, hash-based value numbering, Node::Ideal_DU // and dominator info to a fixed point. - void optimize(); + // When deep is true, after the main worklist drains, re-process + // nodes that inspect the graph deeply (Load, CmpP, If, RangeCheck, + // CountedLoopEnd, LongCountedLoopEnd) to catch optimization opportunities + // from changes far away that the normal notification mechanism misses. + void optimize(bool deep = false); + #ifdef ASSERT - void verify_optimize(); + void verify_optimize(bool deep_revisit_converged); void verify_Value_for(const Node* n, bool strict = false); - void verify_Ideal_for(Node* n, bool can_reshape); + void verify_Ideal_for(Node* n, bool can_reshape, bool deep_revisit_converged); void verify_Identity_for(Node* n); void verify_node_invariants_for(const Node* n); void verify_empty_worklist(Node* n); @@ -510,7 +554,7 @@ class PhaseIterGVN : public PhaseGVN { #ifndef PRODUCT void trace_PhaseIterGVN(Node* n, Node* nn, const Type* old_type); void init_verifyPhaseIterGVN(); - void verify_PhaseIterGVN(); + void verify_PhaseIterGVN(bool deep_revisit_converged); #endif #ifdef ASSERT @@ -526,15 +570,21 @@ class PhaseIterGVN : public PhaseGVN { // It is significant only for debugging and profiling. Node* register_new_node_with_optimizer(Node* n, Node* orig = nullptr); - // Kill a globally dead Node. All uses are also globally dead and are + // Origin of a dead node, describing why it is dying. + // Speculative: a temporarily created node that was never part of the graph + // (e.g., a speculative clone in split_if to test constant foldability). + // Its death does not count as progress for convergence tracking. + enum class NodeOrigin { Graph, Speculative }; + + // Kill a globally dead Node. All uses are also globally dead and are // aggressively trimmed. - void remove_globally_dead_node( Node *dead ); + void remove_globally_dead_node(Node* dead, NodeOrigin origin); // Kill all inputs to a dead node, recursively making more dead nodes. // The Node must be dead locally, i.e., have no uses. - void remove_dead_node( Node *dead ) { + void remove_dead_node(Node* dead, NodeOrigin origin) { assert(dead->outcnt() == 0 && !dead->is_top(), "node must be dead"); - remove_globally_dead_node(dead); + remove_globally_dead_node(dead, origin); } // Add users of 'n' to worklist diff --git a/src/hotspot/share/opto/split_if.cpp b/src/hotspot/share/opto/split_if.cpp index b27b0553320..e5680e93823 100644 --- a/src/hotspot/share/opto/split_if.cpp +++ b/src/hotspot/share/opto/split_if.cpp @@ -79,7 +79,7 @@ bool PhaseIdealLoop::split_up( Node *n, Node *blk1, Node *blk2 ) { if( split_up( n->in(i), blk1, blk2 ) ) { // Got split recursively and self went dead? if (n->outcnt() == 0) - _igvn.remove_dead_node(n); + _igvn.remove_dead_node(n, PhaseIterGVN::NodeOrigin::Graph); return true; } } @@ -257,7 +257,7 @@ void PhaseIdealLoop::clone_loadklass_nodes_at_cmp_index(const Node* n, Node* cmp _igvn.replace_input_of(decode_clone, 1, loadklass_clone); _igvn.replace_input_of(loadklass_clone, MemNode::Address, addp_clone); if (decode->outcnt() == 0) { - _igvn.remove_dead_node(decode); + _igvn.remove_dead_node(decode, PhaseIterGVN::NodeOrigin::Graph); } } } @@ -274,7 +274,7 @@ void PhaseIdealLoop::clone_loadklass_nodes_at_cmp_index(const Node* n, Node* cmp _igvn.replace_input_of(cmp, i, loadklass_clone); _igvn.replace_input_of(loadklass_clone, MemNode::Address, addp_clone); if (loadklass->outcnt() == 0) { - _igvn.remove_dead_node(loadklass); + _igvn.remove_dead_node(loadklass, PhaseIterGVN::NodeOrigin::Graph); } } } @@ -348,7 +348,7 @@ bool PhaseIdealLoop::clone_cmp_down(Node* n, const Node* blk1, const Node* blk2) _igvn.replace_input_of(x2, 1, x1); _igvn.replace_input_of(iff, 1, x2); } - _igvn.remove_dead_node(u); + _igvn.remove_dead_node(u, PhaseIterGVN::NodeOrigin::Graph); --j; } else { // We might see an Opaque1 from a loop limit check here @@ -364,7 +364,7 @@ bool PhaseIdealLoop::clone_cmp_down(Node* n, const Node* blk1, const Node* blk2) --j; } } - _igvn.remove_dead_node(bol); + _igvn.remove_dead_node(bol, PhaseIterGVN::NodeOrigin::Graph); --i; } } @@ -382,7 +382,7 @@ bool PhaseIdealLoop::clone_cmp_down(Node* n, const Node* blk1, const Node* blk2) register_new_node(x, ctrl_or_self(use)); _igvn.replace_input_of(use, pos, x); } - _igvn.remove_dead_node(n); + _igvn.remove_dead_node(n, PhaseIterGVN::NodeOrigin::Graph); return true; } @@ -480,7 +480,7 @@ Node *PhaseIdealLoop::spinup( Node *iff_dom, Node *new_false, Node *new_true, No Node *t = _igvn.hash_find_insert(phi_post); if( t ) { // See if we already have this one // phi_post will not be used, so kill it - _igvn.remove_dead_node(phi_post); + _igvn.remove_dead_node(phi_post, PhaseIterGVN::NodeOrigin::Speculative); phi_post->destruct(&_igvn); phi_post = t; } else { @@ -610,7 +610,7 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio Node* m = n->out(j); // If m is dead, throw it away, and declare progress if (_loop_or_ctrl[m->_idx] == nullptr) { - _igvn.remove_dead_node(m); + _igvn.remove_dead_node(m, PhaseIterGVN::NodeOrigin::Graph); // fall through } else if (m != iff && split_up(m, region, iff)) { @@ -667,7 +667,7 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio new_true = ifpx; } } - _igvn.remove_dead_node(new_iff); + _igvn.remove_dead_node(new_iff, PhaseIterGVN::NodeOrigin::Speculative); // Lazy replace IDOM info with the region's dominator lazy_replace(iff, region_dom); lazy_update(region, region_dom); // idom must be update before handle_uses @@ -682,7 +682,7 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio for (DUIterator k = region->outs(); region->has_out(k); k++) { Node* phi = region->out(k); if (!phi->in(0)) { // Dead phi? Remove it - _igvn.remove_dead_node(phi); + _igvn.remove_dead_node(phi, PhaseIterGVN::NodeOrigin::Graph); } else if (phi == region) { // Found the self-reference continue; // No roll-back of DUIterator } else if (phi->is_Phi()) { // Expected common case: Phi hanging off of Region @@ -701,7 +701,7 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio handle_use(use, phi, &phi_cache, region_dom, new_false, new_true, old_false, old_true); } // End of while phi has uses // Remove the dead Phi - _igvn.remove_dead_node( phi ); + _igvn.remove_dead_node(phi, PhaseIterGVN::NodeOrigin::Graph); } else { assert(phi->in(0) == region, "Inconsistent graph"); // Random memory op guarded by Region. Compute new DEF for USE. @@ -714,7 +714,7 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio --k; } // End of while merge point has phis - _igvn.remove_dead_node(region); + _igvn.remove_dead_node(region, PhaseIterGVN::NodeOrigin::Graph); if (iff->Opcode() == Op_RangeCheck) { // Pin array access nodes: control is updated here to a region. If, after some transformations, only one path // into the region is left, an array load could become dependent on a condition that's not a range check for diff --git a/test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java b/test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java index a0da0741cad..22bf1f88ae7 100644 --- a/test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java +++ b/test/hotspot/jtreg/compiler/arraycopy/TestCloneAccess.java @@ -23,16 +23,18 @@ /* * @test - * @bug 8248791 + * @bug 8248791 8375442 * @summary Test cloning with more than 8 (=ArrayCopyLoadStoreMaxElem) where loads are wrongly replaced by zero. * @requires vm.compiler2.enabled | vm.graal.enabled * * @run main/othervm -XX:-ReduceBulkZeroing - * -XX:CompileCommand=dontinline,compiler.arraycopy.TestCloneAccess::* - * compiler.arraycopy.TestCloneAccess + * -XX:CompileCommand=dontinline,${test.main.class}::* + * ${test.main.class} * @run main/othervm -XX:-ReduceBulkZeroing -XX:-ReduceInitialCardMarks - * -XX:CompileCommand=dontinline,compiler.arraycopy.TestCloneAccess::* - * compiler.arraycopy.TestCloneAccess + * -XX:CompileCommand=dontinline,${test.main.class}::* + * ${test.main.class} + * @run main/othervm -XX:-ReduceBulkZeroing -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + * ${test.main.class} */ package compiler.arraycopy; diff --git a/test/hotspot/jtreg/compiler/igvn/TestFoldComparesCleanup.java b/test/hotspot/jtreg/compiler/igvn/TestFoldComparesCleanup.java new file mode 100644 index 00000000000..bcc3b9541ba --- /dev/null +++ b/test/hotspot/jtreg/compiler/igvn/TestFoldComparesCleanup.java @@ -0,0 +1,56 @@ +/* + * Copyright Amazon.com Inc. or its affiliates. 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 8375442 + * @summary fold_compares_helper must clean up speculative lo node when bailing out with deep revisit + * @library /test/lib / + * @run main/othervm -XX:-TieredCompilation -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:VerifyIterativeGVN=1110 + * -XX:CompileCommand=compileonly,${test.main.class}::test + * ${test.main.class} + * + * @run main ${test.main.class} + */ +package compiler.igvn; + +import jdk.test.lib.Asserts; + +public class TestFoldComparesCleanup { + // Constants chosen so that fold_compares_helper computes adjusted_lim which overflows negative. + static final int A = -2_000_000_000; + static final int B = 2_000_000_000; + + static int test(int z) { + int sum = 0; + if (z > A) sum += 1; + if (z < B) sum += 2; + return sum; + } + + public static void main(String[] args) { + for (int i = 0; i < 50_000; i++) { + Asserts.assertEquals(3, test(i)); + } + } +} diff --git a/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java b/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java index f41ccd84071..40e16b490c6 100644 --- a/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java +++ b/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java @@ -1662,6 +1662,11 @@ public class IRNode { trapNodes(PREDICATE_TRAP, "predicate"); } + public static final String RANGE_CHECK = PREFIX + "RANGE_CHECK" + POSTFIX; + static { + beforeMatchingNameRegex(RANGE_CHECK, "RangeCheck"); + } + public static final String RANGE_CHECK_TRAP = PREFIX + "RANGE_CHECK_TRAP" + POSTFIX; static { trapNodes(RANGE_CHECK_TRAP, "range_check"); diff --git a/test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/TestDeepIGVNRevisit.java b/test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/TestDeepIGVNRevisit.java new file mode 100644 index 00000000000..228357f953d --- /dev/null +++ b/test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/TestDeepIGVNRevisit.java @@ -0,0 +1,83 @@ +/* + * Copyright Amazon.com Inc. or its affiliates. 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. + */ +package compiler.c2.igvn; + +import compiler.lib.ir_framework.*; + +/* + * @test + * @bug 8375442 + * @summary Test deep IGVN revisit for RangeCheck elimination. Other deep-revisit node types + * (If, Load, CmpP, CountedLoopEnd, LongCountedLoopEnd) benefit in large methods + * but require graph complexity beyond this test. + * @library /test/lib / + * @run driver ${test.main.class} + */ +public class TestDeepIGVNRevisit { + static boolean c1, c2, c3, c4; + static volatile int volatileField; + + public static void main(String[] args) { + TestFramework tf = new TestFramework(); + tf.setDefaultWarmup(0); + tf.addFlags("-XX:+IgnoreUnrecognizedVMOptions", + "-XX:+AlwaysIncrementalInline", + "-XX:-PartialPeelLoop", + "-XX:-LoopUnswitching"); + tf.addScenarios( + new Scenario(1, "-XX:-StressIGVN", "-XX:+UseDeepIGVNRevisit"), + new Scenario(2, "-XX:+StressIGVN", "-XX:+UseDeepIGVNRevisit"), + new Scenario(3, "-XX:-StressIGVN", "-XX:-UseDeepIGVNRevisit"), + new Scenario(4, "-XX:+StressIGVN", "-XX:-UseDeepIGVNRevisit")); + tf.start(); + } + + static void lateInline() {} + + // Deferred calls create separate LoadRange nodes for the two arr[idx] + // accesses. After inlining, LoadRanges CSE but RangeCheck#2 is already + // processed. Deep revisit re-processes it with matching range pointers. + @Setup + static Object[] setupRangeCheck() { + return new Object[] { new int[100], 42 }; + } + + @Test + @Arguments(setup = "setupRangeCheck") + @IR(phase = CompilePhase.ITER_GVN2, + applyIf = {"UseDeepIGVNRevisit", "true"}, + counts = {IRNode.RANGE_CHECK, "1"}) + @IR(phase = CompilePhase.ITER_GVN2, + applyIf = {"UseDeepIGVNRevisit", "false"}, + counts = {IRNode.RANGE_CHECK, "2"}) + static int testRangeCheck(int[] arr, int idx) { + int r = arr[idx]; // RangeCheck #1 + if (c1) { lateInline(); } + if (c2) { lateInline(); } + if (c3) { lateInline(); } + if (c4) { lateInline(); } + volatileField = r; + r += arr[idx]; // RangeCheck #2 + return r; + } +}