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

[Christian Hagedorn]

Thank you Tobias for your review!

Best regards,
Christian

On 10.12.19 10:29, Tobias Hartmann wrote:
> 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