RFR: 8251544: CTW: C2 fails with assert(no_dead_loop) failed: dead loop detected

[Vladimir Kozlov]

On Wed, 30 Sep 2020 07:44:06 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> In the testcase, we hit a dead data loop while dead nodes are being removed on a dead control path. A region node, lets
> say `r`, that represents a loop (has three inputs: self, a loop entry and backedge) but is not a loop node, yet,
> becomes dead when its entry control is replaced by top in the first IGVN run after parsing. All its phi nodes also
> become dead by replacing the corresponding entry control input by top. The problem is now that some phi nodes of `r`
> are processed by IGVN before the corresponding (dead) region node `r`. In `PhiNode::Ideal`, we actually check if there
> is a dead loop. But after some of the phis of `r` were already removed, `is_unsafe_data_reference()` on
> [L1939](https://github.com/openjdk/jdk/compare/master...chhagedorn:JDK-8251544#diff-efe6b3bde157b833249cd9a8d8b6645bL1939)
> returns false. As a result, we do not realize in `PhiNode::Ideal` for one of the remaining phis that it is actually
> dead and we apply the normal propagation in `PhiNode::Identity` which replaces the phi by its only non-top input. We
> later apply an additional optimization for a `LoadNode` input of an `AddINode` in which we replace the `LoadNode` by
> the `AddINode` itself (because of the data loop) and we end up with a dead data loop and fail with the dead loop
> assertion.  The order in which the nodes are processed in IGVN is crucial. I could only reproduce this bug with a very
> specific CTW-like testcase which makes this quite an edge case. I can think of two ways ways how to fix this:  1. Delay
> phi nodes which have only one non-top input left and whose region node is not a loop node, has three inputs from which
> the entry control is top and the region has not been processed by IGVN, yet. 2. Extend the dead data loop check in
> `PhiNode::Ideal()` to already do an unreachable region check as done in `RegionNode::Ideal()`. The result can be cached
> as a region should not become reachable anymore once we figured out it is dead.  I chose the second approach because I
> think it is preferable as we are not delaying IGVN and all other phi nodes can already use the information of a dead
> region before it is processed. This avoid any further unwanted optimizations on dead nodes.  Thanks, Christian

Seems reasonable fix.

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

Marked as reviewed by kvn (Reviewer).

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