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

[Tobias Hartmann]

Hi Christian,

as we've discussed offline, this looks good to me. Although the change is quite complex, I think we
should target JDK 14 for the reasons that Vladimir already brought up. Also, the entry->outcnt() > 1
condition makes sure this code is only executed in very specific/rare cases.

Nevertheless, it would be good to get more reviews.

Best regards,
Tobias

On 28.11.19 10:24, Christian Hagedorn wrote:
> 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