[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

[Christian Hagedorn]

Thank you Nils for the review!

Then let's go with option b). I filed an RFE to do a proper fix in 15 [1].

Best regards,
Christian


[1] https://bugs.openjdk.java.net/browse/JDK-8236722

On 07.01.20 15:59, Nils Eliasson wrote:
> 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
>