[jdk17] RFR: 8269820: C2 PhaseIdealLoop::do_unroll get wrong opaque node

[Hui Shi]

On Tue, 20 Jul 2021 08:21:51 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> More discussion in previous PR(https://github.com/openjdk/jdk17/pull/208) is confilict with fix for JDK-8269752.
>> 
>> Opaque1 in main loop entry is expected only used in compare node's second input. However split if might clone opaque1 and replace original node with phi of opaque1 node. This causes assertion in CountedLoopNode::is_canonical_loop_entry now.
>> 
>> Fix is in BoolNode::Ideal, avoiding switching compare node's input when second input is phi of opaque1.
>> 
>> Test: Linux X64 tier1/2/3 release/fastdebug no regression.
>
> The proposed fix is ok but I wonder how robust it is. Are we sure there can't be 2 (or more) split thru phi in a row and that the Opaque1 node is not hiding behind a chain of Phis?
> 
> Note that the bug occurs because the Opaque1 node has control above the predicates due to this:
> https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356
> That logic is not required for Opaque1 nodes AFAICT. So maybe a simpler fix would be to skip it for Opaque1 nodes.

@rwestrel Thanks for you review!

> The proposed fix is ok but I wonder how robust it is. Are we sure there can't be 2 (or more) split thru phi in a row and that the Opaque1 node is not hiding behind a chain of Phis?
> 

In normal case, opaque1 has only 1 input, it cannot split more than one time. In special case, opaque1 has two limit inputs, extra one is original loop limit for range check. But this step(IdealLoopTree::iteration_split) happens after split if, so there will not be cases opaque1 behind a chain of Phis in my own understanding. .

> Note that the bug occurs because the Opaque1 node has control above the predicates due to this:
> https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356
> That logic is not required for Opaque1 nodes AFAICT. So maybe a simpler fix would be to skip it for Opaque1 nodes.

Not understand how https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356 cause this bug.
Are you suggesting skipping split if wit Opaque1 in tree as discussed in ealier PR?  Skipping must be a safer  and conservertive fix, I can submit a new fix if safer fix is perfer now.

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

PR: https://git.openjdk.java.net/jdk17/pull/255