[14] 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

[Nils Eliasson]

Hi Christian,

I agree with Tobias - lets go with fix b for 14, it seems to have the 
lowest risk.

Reviewed.

Best Regards,
Nils

On 2019-12-23 14:12, Tobias Hartmann wrote:
> Hi Christian,
>
> version b) looks good to me and I would go with that for JDK 14.
>
> Best regards,
> Tobias
>
> On 23.12.19 10:30, Christian Hagedorn wrote:
>> Hi
>>
>> Please review the following options for:
>> https://bugs.openjdk.java.net/browse/JDK-8235984
>>
>> The problem:
>> The original fix for JDK-8233033 [1] assumed that partially peeled statements always have a
>> dependency inside the loop to be unswitched (i.e. when following their outputs eventually a phi node
>> is hit belonging to either the slow or the fast loop). However, that is not always the case. As a
>> result the assert was hit.
>>
>> I suggest the following options:
>>
>> Option a) http://cr.openjdk.java.net/~chagedorn/8235984/webrev.a.00/
>> We first check at the start of loop unswitching if there are loop predicates for the loop to be
>> unswitched and if they have an additional control dependency to partially peeled statements (outcnt
>>> 1). Then we explicitly check the assumption that partially peeled statements have a dependency in
>> the loop to be unswitched (in order to keep the fix for JDK-8233033). If that is not the case, we
>> bailout. We could then file an RFE for JDK-15 to handle this missing case properly and remove the
>> bailout.
>>
>> Option b) http://cr.openjdk.java.net/~chagedorn/8235984/webrev.b.00/
>> We only check at the start of loop unswitching if there are loop predicates for the loop to be
>> unswitched and if they have an additional control dependency to partially peeled statements (outcnt
>>> 1). If that's the case we bailout without having the fix from JDK-8233033. We could then file an
>> RFE for JDK-15 to properly handle partially peeled statements all together (kinda a REDO of the fix
>> for JDK-8233033).
>>
>> Option c)
>> Trying to fix the missing cases from JDK-8233033 for JDK-14 without a bailout.
>>
>>
>> I've tried to come up with a fix (option c) last week but without success so far. The idea was to
>> also clone the partially peeled statements without a dependency in the loop to be unswitched, change
>> their control input to the correct cloned predicates and then add a phi node on each loop exit,
>> where we merge the slow and fast loop, to select the correct value. However, this has not worked
>> properly, yet and also involves a higher risk due to its complexity. I think we should not target
>> that option for JDK-14 but do it for JDK-15 in an RFE.
>>
>> Thus, I'd opt for either option a) or b). I tested Tier 1-7 for the complete bailout b) and Tier 1-8
>> for the "conditioned" bailout a). Both look fine. I also ran some standard benchmarks comparing a)
>> and b) to a baseline where I excluded the fix for JDK-8233033 (without bailing out and trying to fix
>> the problem). I could not see any difference in performance. Therefore, it suggests to go with the
>> low risk option b) for JDK-14 and do the entire fix in an RFE for JDK-15.
>>
>> What do you think?
>>
>>
>> Thank you!
>>
>> Best regards,
>> Christian
>>
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8233033