[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

[Tobias Hartmann]

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