[14] RFR(M): 8233033: Results of execution differ for C2 and C1/Xint

[Christian Hagedorn]

Hi

Please review the following patch:
https://bugs.openjdk.java.net/browse/JDK-8233033
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.

This bug can also be reproduced with JDK 11. Should we target this fix 
to 14 or defer it to 15 (since it's a more complex one)?


Thank you!

Best regards,
Christian


[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