diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp index c7f0fb9fc32..2d79857b62a 100644 --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -1,5 +1,6 @@ /* * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024, Alibaba Group Holding Limited. 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 @@ -427,23 +428,31 @@ Node *MemNode::Ideal_common(PhaseGVN *phase, bool can_reshape) { // Used by MemNode::find_previous_store to prove that the // control input of a memory operation predates (dominates) // an allocation it wants to look past. -bool MemNode::all_controls_dominate(Node* dom, Node* sub) { - if (dom == nullptr || dom->is_top() || sub == nullptr || sub->is_top()) - return false; // Conservative answer for dead code +// Returns 'DomResult::Dominate' if all control inputs of 'dom' +// dominate 'sub', 'DomResult::NotDominate' if not, +// and 'DomResult::EncounteredDeadCode' if we can't decide due to +// dead code, but at the end of IGVN, we know the definite result +// once the dead code is cleaned up. +Node::DomResult MemNode::maybe_all_controls_dominate(Node* dom, Node* sub) { + if (dom == nullptr || dom->is_top() || sub == nullptr || sub->is_top()) { + return DomResult::EncounteredDeadCode; // Conservative answer for dead code + } // Check 'dom'. Skip Proj and CatchProj nodes. dom = dom->find_exact_control(dom); - if (dom == nullptr || dom->is_top()) - return false; // Conservative answer for dead code + if (dom == nullptr || dom->is_top()) { + return DomResult::EncounteredDeadCode; // Conservative answer for dead code + } if (dom == sub) { // For the case when, for example, 'sub' is Initialize and the original // 'dom' is Proj node of the 'sub'. - return false; + return DomResult::NotDominate; } - if (dom->is_Con() || dom->is_Start() || dom->is_Root() || dom == sub) - return true; + if (dom->is_Con() || dom->is_Start() || dom->is_Root() || dom == sub) { + return DomResult::Dominate; + } // 'dom' dominates 'sub' if its control edge and control edges // of all its inputs dominate or equal to sub's control edge. @@ -457,16 +466,19 @@ bool MemNode::all_controls_dominate(Node* dom, Node* sub) { // Get control edge of 'sub'. Node* orig_sub = sub; sub = sub->find_exact_control(sub->in(0)); - if (sub == nullptr || sub->is_top()) - return false; // Conservative answer for dead code + if (sub == nullptr || sub->is_top()) { + return DomResult::EncounteredDeadCode; // Conservative answer for dead code + } assert(sub->is_CFG(), "expecting control"); - if (sub == dom) - return true; + if (sub == dom) { + return DomResult::Dominate; + } - if (sub->is_Start() || sub->is_Root()) - return false; + if (sub->is_Start() || sub->is_Root()) { + return DomResult::NotDominate; + } { // Check all control edges of 'dom'. @@ -480,41 +492,47 @@ bool MemNode::all_controls_dominate(Node* dom, Node* sub) { for (uint next = 0; next < dom_list.size(); next++) { Node* n = dom_list.at(next); - if (n == orig_sub) - return false; // One of dom's inputs dominated by sub. + if (n == orig_sub) { + return DomResult::NotDominate; // One of dom's inputs dominated by sub. + } if (!n->is_CFG() && n->pinned()) { // Check only own control edge for pinned non-control nodes. n = n->find_exact_control(n->in(0)); - if (n == nullptr || n->is_top()) - return false; // Conservative answer for dead code + if (n == nullptr || n->is_top()) { + return DomResult::EncounteredDeadCode; // Conservative answer for dead code + } assert(n->is_CFG(), "expecting control"); dom_list.push(n); } else if (n->is_Con() || n->is_Start() || n->is_Root()) { only_dominating_controls = true; } else if (n->is_CFG()) { - if (n->dominates(sub, nlist)) + DomResult dom_result = n->dominates(sub, nlist); + if (dom_result == DomResult::Dominate) { only_dominating_controls = true; - else - return false; + } else { + return dom_result; + } } else { // First, own control edge. Node* m = n->find_exact_control(n->in(0)); if (m != nullptr) { - if (m->is_top()) - return false; // Conservative answer for dead code + if (m->is_top()) { + return DomResult::EncounteredDeadCode; // Conservative answer for dead code + } dom_list.push(m); } // Now, the rest of edges. uint cnt = n->req(); for (uint i = 1; i < cnt; i++) { m = n->find_exact_control(n->in(i)); - if (m == nullptr || m->is_top()) + if (m == nullptr || m->is_top()) { continue; + } dom_list.push(m); } } } - return only_dominating_controls; + return only_dominating_controls ? DomResult::Dominate : DomResult::NotDominate; } } @@ -726,16 +744,18 @@ Node* MemNode::find_previous_store(PhaseValues* phase) { } else if (mem->is_Proj() && mem->in(0)->is_Initialize()) { InitializeNode* st_init = mem->in(0)->as_Initialize(); AllocateNode* st_alloc = st_init->allocation(); - if (st_alloc == nullptr) + if (st_alloc == nullptr) { break; // something degenerated + } bool known_identical = false; bool known_independent = false; - if (alloc == st_alloc) + if (alloc == st_alloc) { known_identical = true; - else if (alloc != nullptr) + } else if (alloc != nullptr) { known_independent = true; - else if (all_controls_dominate(this, st_alloc)) + } else if (all_controls_dominate(this, st_alloc)) { known_independent = true; + } if (known_independent) { // The bases are provably independent: Either they are @@ -1566,8 +1586,9 @@ bool LoadNode::can_split_through_phi_base(PhaseGVN* phase) { } if (!mem->is_Phi()) { - if (!MemNode::all_controls_dominate(mem, base->in(0))) + if (!MemNode::all_controls_dominate(mem, base->in(0))) { return false; + } } else if (base->in(0) != mem->in(0)) { if (!MemNode::all_controls_dominate(mem, base->in(0))) { return false; @@ -1658,35 +1679,49 @@ Node* LoadNode::split_through_phi(PhaseGVN* phase, bool ignore_missing_instance_ // Select Region to split through. Node* region; + DomResult dom_result = DomResult::Dominate; if (!base_is_phi) { assert(mem->is_Phi(), "sanity"); region = mem->in(0); // Skip if the region dominates some control edge of the address. - if (!MemNode::all_controls_dominate(address, region)) - return nullptr; + // We will check `dom_result` later. + dom_result = MemNode::maybe_all_controls_dominate(address, region); } else if (!mem->is_Phi()) { assert(base_is_phi, "sanity"); region = base->in(0); // Skip if the region dominates some control edge of the memory. - if (!MemNode::all_controls_dominate(mem, region)) - return nullptr; + // We will check `dom_result` later. + dom_result = MemNode::maybe_all_controls_dominate(mem, region); } else if (base->in(0) != mem->in(0)) { assert(base_is_phi && mem->is_Phi(), "sanity"); - if (MemNode::all_controls_dominate(mem, base->in(0))) { + dom_result = MemNode::maybe_all_controls_dominate(mem, base->in(0)); + if (dom_result == DomResult::Dominate) { region = base->in(0); - } else if (MemNode::all_controls_dominate(address, mem->in(0))) { - region = mem->in(0); } else { - return nullptr; // complex graph + dom_result = MemNode::maybe_all_controls_dominate(address, mem->in(0)); + if (dom_result == DomResult::Dominate) { + region = mem->in(0); + } + // Otherwise we encountered a complex graph. } } else { assert(base->in(0) == mem->in(0), "sanity"); region = mem->in(0); } + PhaseIterGVN* igvn = phase->is_IterGVN(); + if (dom_result != DomResult::Dominate) { + if (dom_result == DomResult::EncounteredDeadCode) { + // There is some dead code which eventually will be removed in IGVN. + // Once this is the case, we get an unambiguous dominance result. + // Push the node to the worklist again until the dead code is removed. + igvn->_worklist.push(this); + } + return nullptr; + } + Node* phi = nullptr; const Type* this_type = this->bottom_type(); - PhaseIterGVN* igvn = phase->is_IterGVN(); if (t_oop != nullptr && (t_oop->is_known_instance_field() || load_boxed_values)) { int this_index = C->get_alias_index(t_oop); int this_offset = t_oop->offset(); @@ -4571,8 +4606,9 @@ bool InitializeNode::detect_init_independence(Node* value, PhaseGVN* phase) { // must have preceded the init, or else be equal to the init. // Even after loop optimizations (which might change control edges) // a store is never pinned *before* the availability of its inputs. - if (!MemNode::all_controls_dominate(n, this)) + if (!MemNode::all_controls_dominate(n, this)) { return false; // failed to prove a good control + } } // Check data edges for possible dependencies on 'this'. diff --git a/src/hotspot/share/opto/memnode.hpp b/src/hotspot/share/opto/memnode.hpp index 1b65585f1a0..85d206749f6 100644 --- a/src/hotspot/share/opto/memnode.hpp +++ b/src/hotspot/share/opto/memnode.hpp @@ -1,5 +1,6 @@ /* * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024, Alibaba Group Holding Limited. 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 @@ -105,8 +106,12 @@ public: static Node *optimize_simple_memory_chain(Node *mchain, const TypeOopPtr *t_oop, Node *load, PhaseGVN *phase); static Node *optimize_memory_chain(Node *mchain, const TypePtr *t_adr, Node *load, PhaseGVN *phase); - // This one should probably be a phase-specific function: - static bool all_controls_dominate(Node* dom, Node* sub); + // The following two should probably be phase-specific functions: + static DomResult maybe_all_controls_dominate(Node* dom, Node* sub); + static bool all_controls_dominate(Node* dom, Node* sub) { + DomResult dom_result = maybe_all_controls_dominate(dom, sub); + return dom_result == DomResult::Dominate; + } virtual const class TypePtr *adr_type() const; // returns bottom_type of address diff --git a/src/hotspot/share/opto/node.cpp b/src/hotspot/share/opto/node.cpp index f36770e67ea..2e586de33a3 100644 --- a/src/hotspot/share/opto/node.cpp +++ b/src/hotspot/share/opto/node.cpp @@ -1,5 +1,6 @@ /* * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024, Alibaba Group Holding Limited. 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 @@ -1249,7 +1250,7 @@ Node* Node::find_exact_control(Node* ctrl) { // We already know that if any path back to Root or Start reaches 'this', // then all paths so, so this is a simple search for one example, // not an exhaustive search for a counterexample. -bool Node::dominates(Node* sub, Node_List &nlist) { +Node::DomResult Node::dominates(Node* sub, Node_List &nlist) { assert(this->is_CFG(), "expecting control"); assert(sub != nullptr && sub->is_CFG(), "expecting control"); @@ -1269,12 +1270,15 @@ bool Node::dominates(Node* sub, Node_List &nlist) { // will either exit through the loop head, or give up. // (If we get confused, break out and return a conservative 'false'.) while (sub != nullptr) { - if (sub->is_top()) break; // Conservative answer for dead code. + if (sub->is_top()) { + // Conservative answer for dead code. + return DomResult::EncounteredDeadCode; + } if (sub == dom) { if (nlist.size() == 0) { // No Region nodes except loops were visited before and the EntryControl // path was taken for loops: it did not walk in a cycle. - return true; + return DomResult::Dominate; } else if (met_dom) { break; // already met before: walk in a cycle } else { @@ -1288,7 +1292,7 @@ bool Node::dominates(Node* sub, Node_List &nlist) { // Success if we met 'dom' along a path to Start or Root. // We assume there are no alternative paths that avoid 'dom'. // (This assumption is up to the caller to ensure!) - return met_dom; + return met_dom ? DomResult::Dominate : DomResult::NotDominate; } Node* up = sub->in(0); // Normalize simple pass-through regions and projections: @@ -1319,7 +1323,7 @@ bool Node::dominates(Node* sub, Node_List &nlist) { if (visited == sub) { if (visited_twice_already) { // Visited 2 paths, but still stuck in loop body. Give up. - return false; + return DomResult::NotDominate; } // The Region node was visited before only once. // (We will repush with the low bit set, below.) @@ -1362,8 +1366,7 @@ bool Node::dominates(Node* sub, Node_List &nlist) { } // Did not meet Root or Start node in pred. chain. - // Conservative answer for dead code. - return false; + return DomResult::NotDominate; } //------------------------------remove_dead_region----------------------------- diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 3e39e1ed2fb..bc5842866a1 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -1,5 +1,6 @@ /* * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024, Alibaba Group Holding Limited. 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 @@ -1107,8 +1108,14 @@ public: // Skip Proj and CatchProj nodes chains. Check for Null and Top. Node* find_exact_control(Node* ctrl); + // Results of the dominance analysis. + enum class DomResult { + NotDominate, // 'this' node does not dominate 'sub'. + Dominate, // 'this' node dominates or is equal to 'sub'. + EncounteredDeadCode // Result is undefined due to encountering dead code. + }; // Check if 'this' node dominates or equal to 'sub'. - bool dominates(Node* sub, Node_List &nlist); + DomResult dominates(Node* sub, Node_List &nlist); protected: bool remove_dead_region(PhaseGVN *phase, bool can_reshape); diff --git a/test/hotspot/jtreg/compiler/c2/irTests/scalarReplacement/ScalarReplacementWithGCBarrierTests.java b/test/hotspot/jtreg/compiler/c2/irTests/scalarReplacement/ScalarReplacementWithGCBarrierTests.java new file mode 100644 index 00000000000..fbf5cdd61cc --- /dev/null +++ b/test/hotspot/jtreg/compiler/c2/irTests/scalarReplacement/ScalarReplacementWithGCBarrierTests.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2024 Alibaba Group Holding Limited. 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.irTests.scalarReplacement; + +import compiler.lib.ir_framework.*; + +/* + * @test + * @bug 8333334 + * @summary Tests that dead barrier control flows do not affect the scalar replacement. + * @library /test/lib / + * @requires vm.compiler2.enabled + * @requires vm.gc.G1 + * @run driver compiler.c2.irTests.scalarReplacement.ScalarReplacementWithGCBarrierTests + */ +public class ScalarReplacementWithGCBarrierTests { + static class List { + public Node head; + + public void push(int value) { + Node n = new Node(); + n.value = value; + n.next = head; + head = n; + } + + @ForceInline + public Iter iter() { + Iter iter = new Iter(); + iter.list = this; + iter.n = head; + iter.sum = 0; + return iter; + } + } + + static class Node { + public int value; + public Node next; + } + + static class Iter { + public List list; + public Node n; + public Integer sum; + + @ForceInline + public boolean next() { + int lastSum = sum; + while (sum - lastSum < 1000) { + while (n != null && n.value < 30) n = n.next; + if (n == null) return false; + sum += n.value; + n = n.next; + } + return true; + } + } + + private static final int SIZE = 1000; + + public static void main(String[] args) { + // Must use G1 GC to ensure there is a pre-barrier + // before the first field write. + TestFramework.runWithFlags("-XX:+UseG1GC"); + } + + @Run(test = "testScalarReplacementWithGCBarrier") + private void runner() { + List list = new List(); + for (int i = 0; i < SIZE; i++) { + list.push(i); + } + testScalarReplacementWithGCBarrier(list); + } + + // Allocation of `Iter iter` should be eliminated by scalar replacement, and + // the allocation of `Integer sum` can not be eliminated, so there should be + // 1 allocation after allocations and locks elimination. + // + // Before the patch of JDK-8333334, both allocations of `Iter` and `Integer` + // could not be eliminated. + @Test + @IR(phase = { CompilePhase.AFTER_PARSING }, counts = { IRNode.ALLOC, "1" }) + @IR(phase = { CompilePhase.INCREMENTAL_BOXING_INLINE }, counts = { IRNode.ALLOC, "2" }) + @IR(phase = { CompilePhase.ITER_GVN_AFTER_ELIMINATION }, counts = { IRNode.ALLOC, "1" }) + private int testScalarReplacementWithGCBarrier(List list) { + Iter iter = list.iter(); + while (true) { + while (iter.next()) {} + if (list.head == null) break; + list.head = list.head.next; + iter.n = list.head; + } + return iter.sum; + } +} diff --git a/test/micro/org/openjdk/bench/java/util/concurrent/Maps.java b/test/micro/org/openjdk/bench/java/util/concurrent/Maps.java index bd68e582e6a..62883efb8bb 100644 --- a/test/micro/org/openjdk/bench/java/util/concurrent/Maps.java +++ b/test/micro/org/openjdk/bench/java/util/concurrent/Maps.java @@ -1,5 +1,6 @@ /* * Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024, Alibaba Group Holding Limited. 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 @@ -35,6 +36,7 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Threads; import org.openjdk.jmh.annotations.Warmup; +import java.util.Enumeration; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; @@ -127,6 +129,21 @@ public class Maps { return map; } + @Benchmark + public int testConcurrentHashMapIterators() { + ConcurrentHashMap map = (ConcurrentHashMap) staticMap; + int sum = 0; + Enumeration it = map.elements(); + while (it.hasMoreElements()) { + sum += (int) it.nextElement(); + } + it = map.keys(); + while (it.hasMoreElements()) { + sum += (int) it.nextElement(); + } + return sum; + } + private static class SimpleRandom { private final static long multiplier = 0x5DEECE66DL; private final static long addend = 0xBL;