Skip to content

Commit 54bbe76

Browse files
committedOct 12, 2020
8251544: CTW: C2 fails with assert(no_dead_loop) failed: dead loop detected
Reviewed-by: kvn, roland
1 parent 13fe054 commit 54bbe76

File tree

3 files changed

+253
-31
lines changed

3 files changed

+253
-31
lines changed
 

‎src/hotspot/share/opto/cfgnode.cpp

+51-22
Original file line numberDiff line numberDiff line change
@@ -315,37 +315,60 @@ static bool check_compare_clipping( bool less_than, IfNode *iff, ConNode *limit,
315315

316316
//------------------------------is_unreachable_region--------------------------
317317
// Find if the Region node is reachable from the root.
318-
bool RegionNode::is_unreachable_region(PhaseGVN *phase) const {
319-
assert(req() == 2, "");
318+
bool RegionNode::is_unreachable_region(const PhaseGVN* phase) {
319+
Node* top = phase->C->top();
320+
assert(req() == 2 || (req() == 3 && in(1) != NULL && in(2) == top), "sanity check arguments");
321+
if (_is_unreachable_region) {
322+
// Return cached result from previous evaluation which should still be valid
323+
assert(is_unreachable_from_root(phase), "walk the graph again and check if its indeed unreachable");
324+
return true;
325+
}
320326

321327
// First, cut the simple case of fallthrough region when NONE of
322328
// region's phis references itself directly or through a data node.
329+
if (is_possible_unsafe_loop(phase)) {
330+
// If we have a possible unsafe loop, check if the region node is actually unreachable from root.
331+
if (is_unreachable_from_root(phase)) {
332+
_is_unreachable_region = true;
333+
return true;
334+
}
335+
}
336+
return false;
337+
}
338+
339+
bool RegionNode::is_possible_unsafe_loop(const PhaseGVN* phase) const {
323340
uint max = outcnt();
324341
uint i;
325342
for (i = 0; i < max; i++) {
326-
Node* phi = raw_out(i);
327-
if (phi != NULL && phi->is_Phi()) {
328-
assert(phase->eqv(phi->in(0), this) && phi->req() == 2, "");
329-
if (phi->outcnt() == 0)
343+
Node* n = raw_out(i);
344+
if (n != NULL && n->is_Phi()) {
345+
PhiNode* phi = n->as_Phi();
346+
assert(phase->eqv(phi->in(0), this), "sanity check phi");
347+
if (phi->outcnt() == 0) {
330348
continue; // Safe case - no loops
349+
}
331350
if (phi->outcnt() == 1) {
332351
Node* u = phi->raw_out(0);
333352
// Skip if only one use is an other Phi or Call or Uncommon trap.
334353
// It is safe to consider this case as fallthrough.
335-
if (u != NULL && (u->is_Phi() || u->is_CFG()))
354+
if (u != NULL && (u->is_Phi() || u->is_CFG())) {
336355
continue;
356+
}
337357
}
338358
// Check when phi references itself directly or through an other node.
339-
if (phi->as_Phi()->simple_data_loop_check(phi->in(1)) >= PhiNode::Unsafe)
359+
if (phi->as_Phi()->simple_data_loop_check(phi->in(1)) >= PhiNode::Unsafe) {
340360
break; // Found possible unsafe data loop.
361+
}
341362
}
342363
}
343-
if (i >= max)
364+
if (i >= max) {
344365
return false; // An unsafe case was NOT found - don't need graph walk.
366+
}
367+
return true;
368+
}
345369

346-
// Unsafe case - check if the Region node is reachable from root.
370+
bool RegionNode::is_unreachable_from_root(const PhaseGVN* phase) const {
347371
ResourceMark rm;
348-
349372
Node_List nstack;
350373
VectorSet visited;
351374

@@ -367,7 +390,6 @@ bool RegionNode::is_unreachable_region(PhaseGVN *phase) const {
367390
}
368391
}
369392
}
370-
371393
return true; // The Region node is unreachable - it is dead.
372394
}
373395

@@ -1929,16 +1951,7 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
19291951
// Determine if this input is backedge of a loop.
19301952
// (Skip new phis which have no uses and dead regions).
19311953
if (outcnt() > 0 && r->in(0) != NULL) {
1932-
// First, take the short cut when we know it is a loop and
1933-
// the EntryControl data path is dead.
1934-
// Loop node may have only one input because entry path
1935-
// is removed in PhaseIdealLoop::Dominators().
1936-
assert(!r->is_Loop() || r->req() <= 3, "Loop node should have 3 or less inputs");
1937-
bool is_loop = (r->is_Loop() && r->req() == 3);
1938-
// Then, check if there is a data loop when phi references itself directly
1939-
// or through other data nodes.
1940-
if ((is_loop && !uin->eqv_uncast(in(LoopNode::EntryControl))) ||
1941-
(!is_loop && is_unsafe_data_reference(uin))) {
1954+
if (is_data_loop(r->as_Region(), uin, phase)) {
19421955
// Break this data loop to avoid creation of a dead loop.
19431956
if (can_reshape) {
19441957
return top;
@@ -2377,6 +2390,22 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
23772390
return progress; // Return any progress
23782391
}
23792392

2393+
bool PhiNode::is_data_loop(RegionNode* r, Node* uin, const PhaseGVN* phase) {
2394+
// First, take the short cut when we know it is a loop and the EntryControl data path is dead.
2395+
// The loop node may only have one input because the entry path was removed in PhaseIdealLoop::Dominators().
2396+
// Then, check if there is a data loop when the phi references itself directly or through other data nodes.
2397+
assert(!r->is_Loop() || r->req() <= 3, "Loop node should have 3 or less inputs");
2398+
const bool is_loop = (r->is_Loop() && r->req() == 3);
2399+
const Node* top = phase->C->top();
2400+
if (is_loop) {
2401+
return !uin->eqv_uncast(in(LoopNode::EntryControl));
2402+
} else {
2403+
// We have a data loop either with an unsafe data reference or if a region is unreachable.
2404+
return is_unsafe_data_reference(uin)
2405+
|| (r->req() == 3 && (r->in(1) != top && r->in(2) == top && r->is_unreachable_region(phase)));
2406+
}
2407+
}
2408+
23802409
//------------------------------is_tripcount-----------------------------------
23812410
bool PhiNode::is_tripcount() const {
23822411
return (in(0) != NULL && in(0)->is_CountedLoop() &&

‎src/hotspot/share/opto/cfgnode.hpp

+16-9
Original file line numberDiff line numberDiff line change
@@ -64,15 +64,20 @@ class PhaseIdealLoop;
6464
// correspond 1-to-1 with RegionNode inputs. The zero input of a PhiNode is
6565
// the RegionNode, and the zero input of the RegionNode is itself.
6666
class RegionNode : public Node {
67+
private:
68+
bool _is_unreachable_region;
69+
70+
bool is_possible_unsafe_loop(const PhaseGVN* phase) const;
71+
bool is_unreachable_from_root(const PhaseGVN* phase) const;
6772
public:
6873
// Node layout (parallels PhiNode):
6974
enum { Region, // Generally points to self.
7075
Control // Control arcs are [1..len)
7176
};
7277

73-
RegionNode( uint required ) : Node(required) {
78+
RegionNode(uint required) : Node(required), _is_unreachable_region(false) {
7479
init_class_id(Class_Region);
75-
init_req(0,this);
80+
init_req(0, this);
7681
}
7782

7883
Node* is_copy() const {
@@ -84,18 +89,19 @@ class RegionNode : public Node {
8489
PhiNode* has_phi() const; // returns an arbitrary phi user, or NULL
8590
PhiNode* has_unique_phi() const; // returns the unique phi user, or NULL
8691
// Is this region node unreachable from root?
87-
bool is_unreachable_region(PhaseGVN *phase) const;
92+
bool is_unreachable_region(const PhaseGVN* phase);
8893
virtual int Opcode() const;
89-
virtual bool pinned() const { return (const Node *)in(0) == this; }
90-
virtual bool is_CFG () const { return true; }
91-
virtual uint hash() const { return NO_HASH; } // CFG nodes do not hash
94+
virtual uint size_of() const { return sizeof(*this); }
95+
virtual bool pinned() const { return (const Node*)in(0) == this; }
96+
virtual bool is_CFG() const { return true; }
97+
virtual uint hash() const { return NO_HASH; } // CFG nodes do not hash
9298
virtual bool depends_only_on_test() const { return false; }
93-
virtual const Type *bottom_type() const { return Type::CONTROL; }
99+
virtual const Type* bottom_type() const { return Type::CONTROL; }
94100
virtual const Type* Value(PhaseGVN* phase) const;
95101
virtual Node* Identity(PhaseGVN* phase);
96-
virtual Node *Ideal(PhaseGVN *phase, bool can_reshape);
102+
virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
97103
virtual const RegMask &out_RegMask() const;
98-
bool try_clean_mem_phi(PhaseGVN *phase);
104+
bool try_clean_mem_phi(PhaseGVN* phase);
99105
bool optimize_trichotomy(PhaseIterGVN* igvn);
100106
};
101107

@@ -135,6 +141,7 @@ class PhiNode : public TypeNode {
135141
// Determine if CMoveNode::is_cmove_id can be used at this join point.
136142
Node* is_cmove_id(PhaseTransform* phase, int true_path);
137143
bool wait_for_region_igvn(PhaseGVN* phase);
144+
bool is_data_loop(RegionNode* r, Node* uin, const PhaseGVN* phase);
138145

139146
public:
140147
// Node layout (parallels RegionNode):
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
/*
2+
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test
26+
* @bug 8251544
27+
* @summary A dead data loop in dying code is not correctly removed resulting in unremovable data nodes.
28+
* @requires vm.compiler2.enabled
29+
* @library /test/lib /
30+
* @modules java.base/jdk.internal.misc
31+
* java.management
32+
* java.base/jdk.internal.access
33+
* java.base/jdk.internal.reflect
34+
*
35+
* @build sun.hotspot.WhiteBox
36+
* @run driver ClassFileInstaller sun.hotspot.WhiteBox
37+
* @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:-TieredCompilation -Xbatch
38+
* compiler.c2.TestDeadDataLoopIGVN
39+
*/
40+
41+
package compiler.c2;
42+
43+
import java.lang.reflect.Method;
44+
import java.util.HashMap;
45+
import java.util.List;
46+
import java.util.LinkedList;
47+
import java.util.ArrayList;
48+
import java.util.Vector;
49+
import java.util.ListIterator;
50+
import jdk.internal.access.SharedSecrets;
51+
import jdk.internal.misc.Unsafe;
52+
import jdk.internal.reflect.ConstantPool;
53+
import java.net.URL;
54+
import java.net.URLClassLoader;
55+
import java.lang.reflect.Executable;
56+
57+
import compiler.whitebox.CompilerWhiteBoxTest;
58+
import sun.hotspot.WhiteBox;
59+
60+
public class TestDeadDataLoopIGVN {
61+
62+
private static final WhiteBox WB = WhiteBox.getWhiteBox();
63+
private static final int TIERED_STOP_AT_LEVEL = WB.getIntxVMFlag("TieredStopAtLevel").intValue();
64+
private static final Unsafe UNSAFE = Unsafe.getUnsafe();
65+
66+
// The original test only failed with CTW due to different inlining and virtual call decisions compared to an
67+
// execution with -Xcomp. This test adapts the behavior of CTW and compiles the methods with the Whitebox API
68+
// in order to reproduce the bug.
69+
public static void main(String[] strArr) throws Exception {
70+
// Required to get the same inlining/virtual call decisions as for CTW
71+
callSomeMethods(new ArrayList<String>());
72+
callSomeMethods(new Vector<String>());
73+
74+
if (TIERED_STOP_AT_LEVEL != CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION) {
75+
throw new RuntimeException("Sanity check if C2 is available");
76+
}
77+
78+
// To trigger the assertion, we only need to compile Test
79+
compileClass(Test.class);
80+
}
81+
82+
private static void callSomeMethods(List<String> list) {
83+
list.add("bla");
84+
list.add("foo");
85+
ListIterator<String> it = list.listIterator();
86+
it.hasNext();
87+
for (String s : list) {
88+
s.charAt(0);
89+
}
90+
}
91+
92+
// Adaptation from CTW to compile and deoptimize the same methods
93+
private static void compileClass(Class<?> aClass) throws Exception {
94+
aClass = Class.forName(aClass.getCanonicalName(), true, aClass.getClassLoader());
95+
ConstantPool constantPool = SharedSecrets.getJavaLangAccess().getConstantPool(aClass);
96+
preloadClasses(constantPool);
97+
UNSAFE.ensureClassInitialized(aClass);
98+
WB.enqueueInitializerForCompilation(aClass, 4); // Level 4 for C2
99+
100+
for (Executable method : aClass.getDeclaredConstructors()) {
101+
WB.deoptimizeMethod(method);
102+
WB.enqueueMethodForCompilation(method, 4);
103+
WB.deoptimizeMethod(method);
104+
}
105+
106+
for (Executable method : aClass.getDeclaredMethods()) {
107+
WB.deoptimizeMethod(method);
108+
WB.enqueueMethodForCompilation(method, 4);
109+
WB.deoptimizeMethod(method);
110+
}
111+
}
112+
113+
private static void preloadClasses(ConstantPool constantPool) throws Exception {
114+
for (int i = 0, n = constantPool.getSize(); i < n; ++i) {
115+
try {
116+
constantPool.getClassAt(i);
117+
} catch (IllegalArgumentException ignore) {
118+
}
119+
}
120+
}
121+
}
122+
123+
// The actual class that failed by executing it with CTW
124+
class Test {
125+
126+
public static A a = new A();
127+
128+
Test() {
129+
LinkedList<A> l = new LinkedList<A>();
130+
for (int i = 0; i < 34; i++) {
131+
A instance = new A();
132+
instance.id = i;
133+
l.add(instance);
134+
}
135+
test(l, 34);
136+
}
137+
138+
public void test(LinkedList<A> list, int max) {
139+
Integer[] numbers = new Integer[max + 1];
140+
A[] numbers2 = new A[max + 1];
141+
int n = 0;
142+
ListIterator<A> it = list.listIterator();
143+
while (it.hasNext()) {
144+
A b = it.next();
145+
numbers[b.get()] = n;
146+
numbers2[n] = b;
147+
n++;
148+
}
149+
150+
Integer[] iArr = new Integer[max + 1];
151+
152+
A a = getA();
153+
Integer x = numbers[a.get()];
154+
iArr[x] = x;
155+
156+
boolean flag = true;
157+
while (flag) {
158+
flag = false;
159+
it = list.listIterator(34);
160+
while (it.hasPrevious()) {
161+
A b = it.previous();
162+
if (b == a) {
163+
continue;
164+
}
165+
}
166+
}
167+
168+
HashMap<A, A> map = new HashMap<A, A>();
169+
for (Integer i = 0; i < max - 34; i++) {
170+
map.put(numbers2[i], numbers2[iArr[i]]);
171+
}
172+
}
173+
174+
public A getA() {
175+
return a;
176+
}
177+
}
178+
179+
// Helper class
180+
class A {
181+
int id;
182+
183+
public int get() {
184+
return id;
185+
}
186+
}

0 commit comments

Comments
 (0)
Please sign in to comment.