Bug ID: JDK-8233033 C2 produces wrong result while unswitching a loop due to lost control dependencies

Type: Bug
Component: hotspot
Sub-Component: compiler
Affected Version: 11,12,13,14

Priority: P2
Status: Closed
Resolution: Fixed

Submitted: 2019-10-25
Updated: 2022-06-27
Resolved: 2019-12-11

JDK 11	JDK 13	JDK 14	JDK 15
11.0.8-oracleFixed	13.0.4Fixed	14 b27Fixed	15Fixed

The attached test produces a wrong result for C2 due to lost control dependencies for loop predicates while unswitching a loop after it was partially peeled.

The C2 compiled code produces a wrong result for 'iFld' in the attached test case. It is -8 instead of -7. The loop in the test case is partially peeled and then unswitched. The wrong result is produced because a wrong state is transferred to the interpreter when an uncommon trap is hit in the C2 compiled code in the fast version of the unswitched loop.

The problem is when unswitching the loop, we clone the original loop predicates for the slow and fast version of the loop but we do not account for partially peeled statements that are control dependent on the loop predicates (i.e. need to be executed after the predicates). As a result, these are executed before the cloned loop predicates.

Fix request (13u): The original change applies cleanly, passes tier1 and tier2 tests.
03-06-2020
8u backport: the test case doesn't fail with 8u. AFAICT the test case requires UseProfiledLoopPredicate and UseSwitchProfiling both of which don't exist in 8u. I tried transforming the test case (by converting the switch into the tree of ifs it is compiled to) but couldn't get it to fail. The bug is quite possibly there in 8u but as long as we don't have proof, I don't think it's worth backporting.
09-03-2020
11u Fix Request Backporting this patch eliminates a miscompilation with C2 which may result in an incorrect result. Patch applies cleanly to 11u. New test fails without the product patch, and passes with it. tier1 and tier2 tests pass with the patch.
05-03-2020
URL: https://hg.openjdk.java.net/jdk/jdk/rev/63004af6fc57 User: thartmann Date: 2019-12-11 13:57:40 +0000
11-12-2019
http://cr.openjdk.java.net/~chagedorn/8233033/webrev.00/ The C2 compiled code produces a wrong result for 'iFld' in the test case. It is -8 instead of -7. The loop in the test case is partially peeled and then unswitched. The wrong result is produced because a wrong state is transferred to the interpreter when an uncommon trap is hit in the C2 compiled code in the fast version of the unswitched loop. The problem is when unswitching the loop, we clone the original loop predicates for the slow and fast version of the loop [1] but we do not account for partially peeled statements that are control dependent on the loop predicates (i.e. need to be executed after the predicates). As a result, these are executed before the cloned loop predicates. The situation of the test case method PartialPeelingUnswitch::test() is visualized in [2]. IfTrue 118, the entry control of the original loop which follows right after the loop predicates, has an output edge to the StoreI 353 node. This node belongs to the "iFld += -7" statement which was partially peeled before. When creating the slow version of the loop and cloning the predicates in PhaseIdealLoop::create_slow_version_of_loop(), this control dependency is lost. StoreI 353 is still dependent on IfTrue 118 instead of IfTrue 472 (fast loop entry control) and IfTrue 476 (slow loop entry control). The original loop predicates are later removed and thus, when hitting the uncommon trap in the fast loop, we accidentally executed "iFld += -7" (StoreI 353) already even though the interpreter assumes C2 has not executed any statements in the loop. As a result, "iFld += -7" is executed twice in a row which produces a wrong result. The fix is to replace the control input of all statements that have a control input from the original loop entry control (and are not the "loop selection" IfNode) with the fast and slow entry control, respectively. Since the statements cannot have two control inputs they need to be cloned together with all following nodes on a path to the loop phi nodes. The output of the last node before a loop phi on such a path needs to be adjusted to only point to the phi node belonging to the fast loop. The last node on the cloned path is set to the phi node belonging to the slow loop. The fix is visualized in [3]. The control input of StoreI 353 is now the entry control of the fast loop (IfTrue 472) and its output only points to the corresponding Phi 442 of the fast loop. The same was done for the cloned node StoreI 476 of StoreI 353 for the slow loop. [1] http://hg.openjdk.java.net/jdk/jdk/file/6f42d2a19117/src/hotspot/share/opto/loopUnswitch.cpp#l272 [2] https://bugs.openjdk.java.net/secure/attachment/85593/wrong_dependencies.png [3] https://bugs.openjdk.java.net/secure/attachment/85592/fixed_dependencies.png
28-11-2019
Reproduces with JDK 11 but not with JDK 8u
08-11-2019
ILW = Incorrect result of C2 compiled code, reproducible with JavaFuzzer generated test, disable compilation = HMM = P2
29-10-2019
Issue reproduced with JDK 14 b20.
25-10-2019
java -cp . -Xint Test > Def java -cp . -Xmx1G -Xcomp -Xbatch -XX:-TieredCompilation -XX:CompileOnly=Test Test > C217c17 < Test.iFld Test.byFld Test.fArrFld = -7,65,4659694054417629184 --- > Test.iFld Test.byFld Test.fArrFld = -8,65,4659694054417629184 28c28 < Test.iFld Test.byFld Test.fArrFld = -7,-79,4665948456165048320 --- > Test.iFld Test.byFld Test.fArrFld = -8,-79,4665948456165048320 39c39 < Test.iFld Test.byFld Test.fArrFld = -7,33,4665946402121580544 --- > Test.iFld Test.byFld Test.fArrFld = -8,33,4665946402121580544 50c50 < Test.iFld Test.byFld Test.fArrFld = -7,-111,4666320384589037568 --- > Test.iFld Test.byFld Test.fArrFld = -8,-111,4666320384589037568
25-10-2019

Relates :	JDK-8235984 - C2: assert(out->in(PhiNode::Region) == head \|\| out->in(PhiNode::Region) == slow_head) failed: phi must be either part of the slow or the fast loop
Relates :	JDK-8236722 - Extend loop unswitching to handle loop predicates with a control dependency to partially peeled statements