[13] RFR(S): 8224957: C2 compilation fails with assert: Bad graph detected in build_loop_late

[Tobias Hartmann]

Hi,

please review the following patch:
https://bugs.openjdk.java.net/browse/JDK-8224957
http://cr.openjdk.java.net/~thartmann/8224957/webrev.00/

*Background*
The fix for JDK-8217919 [1] re-enabled AggressiveUnboxing by default. LoadNode::split_through_phi
therefore tries to split loads from box value fields through their input memory phis (because these
boxes are immutable). The code is supposed to bail out if the region dominates some control edge of
the address by calling MemNode::all_controls_dominate [2] which relies on Node::dominates.

*Problem*
Node::dominates implicitly assumes that regions with more than two input paths are always dominated
by their first input [3]. This is not correct.

*Details*
In the failing case (see regression test), we OSR compile a loop with two backbranches. After
parsing, Region 84 has not yet been converted to a loop, input nodes 190 and 186 belong to the
backbranches and input 83 is the OSR entry:
http://cr.openjdk.java.net/~thartmann/8224957/1_after_parsing.png

We then attempt to split LoadI 139 through Phi 86 with Region 84 and therefore check if control of
the AddP 138 (which is If 97) dominates that region. Node::dominates walks up the dominator chain
from Region 84 to check if If 97 is reachable. Since the region has more than two input paths, only
the backbranch at in(1) is taken and after reversing through the loop body we finally reach If 97.
Since we did not detect any loops, we incorrectly conclude that If 97 must dominate Region 84 [4].

After splitting LoadI 139 through Phi 86, the graph looks like this:
http://cr.openjdk.java.net/~thartmann/8224957/2_after_split_through_phi.png

We then assert after beautify loops due to a bad graph:
http://cr.openjdk.java.net/~thartmann/8224957/3_after_beautify_loops.png

Phi 200 has Loop 209 as control and LoadI 203 as dependency which can't be scheduled that early
because its address depends on If 97 (see log at [5] for details).

The problematic code was added long ago with the fix for JDK-6695810 [6] but never triggered because
AggressiveUnboxing was disabled. Also, it was very hard to reproduce this problem (only happened
with JMH executing a Java Script benchmark with Nashorn) and even harder to extract a simple test.

*Fix*
My proposed fix simply disables walking up the dominator chain for regions with more than 2 input
paths. I'm currently running correctness and performance testing.

An alternative would be to handle regions with more than 2 inputs paths by visiting all inputs.

Thanks,
Tobias

[1] https://bugs.openjdk.java.net/browse/JDK-8217919
[2] http://hg.openjdk.java.net/jdk/jdk/file/5da01706bf11/src/hotspot/share/opto/memnode.cpp#l1453
[3] http://hg.openjdk.java.net/jdk/jdk/file/5da01706bf11/src/hotspot/share/opto/node.cpp#l1231
[4] http://hg.openjdk.java.net/jdk/jdk/file/5da01706bf11/src/hotspot/share/opto/node.cpp#l1206
[5] http://cr.openjdk.java.net/~thartmann/8224957/fail.log
[6] https://bugs.openjdk.java.net/browse/JDK-6695810