RFR: 8253644: C2: assert(skeleton_predicate_has_opaque(iff)) failed: unexpected

[Christian Hagedorn]

On Fri, 27 Nov 2020 09:00:03 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> > Yes, exactly. So, we unswitch in one round A. Then we have the first loop tree shown with the slow and fast loop at the start of the next round B. In this round B, we apply Split If that is then able to remove the [`IfNode` (2)](https://github.com/chhagedorn/jdk/blob/cfc7c941ddefd716c9f07bf7e8f4824f00bb7e9b/test/hotspot/jtreg/compiler/loopopts/TestUnswitchCloneSkeletonPredicates.java#L101). We always take the true projection ( `x == 100` is true) which means that we immediately exit the fast loop N459 on the first iteration. The `CountedLoopNode` is therefore removed. The second loop tree shown is at the start of the next round C where we then finally apply the peeling and pre/main/post steps which lets the assertion fail. This description of N459 is indeed a bit misleading. When afterwards talking about the fast loop I just mean where the fast loop was before to keep things simpler.
> 
> Thanks for the clarification.
> On round C, N459 is not part of the loop tree anymore then?
> If that's case shouldn't we remove useless skeleton predicates because then don't guard a loop anymore (PhaseIdealLoop::eliminate_useless_predicates())?

I had a look at that method. Apparently, we are only removing `Opaque1` predicates but no `Opaque4` skeleton predicates:
https://github.com/openjdk/jdk/blob/973255c469d794afe8ee74b24ddb5048bfcaadf7/src/hotspot/share/opto/loopnode.cpp#L3573-L3579
Could that be improved? But as of the current implementation, the `Opaque4` node with its inputs is shared between a fast and a slow loop. So we could not remove the `Opaque4` node when either the fast or slow loop does not need it anymore while the other one does.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1448