RFR: 8287217: C2: PhaseCCP: remove not visited nodes, prevent type inconsistency [v2]

[Christian Hagedorn]

On Fri, 16 Sep 2022 14:48:04 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> **Context:**
>> [JDK-8265973](https://bugs.openjdk.org/browse/JDK-8265973) Fix in Valhalla repository (Tobias @TobiHartmann  ).
>> [JDK-8290711](https://bugs.openjdk.org/browse/JDK-8290711) Fix in mainline (Roland @rwestrel ).
>> Tobias' fix is a superset of Rolands. Unfortunately, Tobias gave up his fix once Rolands came up, because they did not have tests that required the superset fix.
>> 
>> We now have such a test, where Rolands fix is not sufficient. But Tobias' fix is the solution. So I ported Tobias' fix to mainline.
>> 
>> **Analysis:**
>> In this bug, we have two `LoadB` nodes re-pushing themselves to the `igvn.worklist`, without end. This leads to an assert after too many iterations.
>> 
>> `PhaseCCP::analyze` is looking at a post-loop. The loop has a memory access, so there is a `null_check`. The data-part of the loop is connected down to Root via this `null_check`
>> (`Phi-> CmpP -> Bool -> If -> IfFalse -> Region -> CallStaticJava uncommon_trap -> ... -> Root`).
>> During `CCP::analyze`, we discover that the memory address is NonNull. So we update the `phase->type(n)` for many of the data-nodes of the loop.
>> 
>> During `PhaseCCP::do_transform`, we now traverse recursively up from the root, visiting all reachable nodes.
>> When we visit a node, we store the cached `phase->type(n)` into the node, making the node's type consistent.
>> We traverse up through the `null_check`, through the `uncommon_trap`, and the `If`, to the `Bool` node.
>> `BoolNode::Value` realizes that we can never have Null, and is subsumed by constant `#int:1` (true).
>> This means that the data-part of the loop just lost its connection down to Root.
>> The traversal now also does not reach further than the Bool node which was just subsumed, and hence does not reach the data-part of the loop.
>> This means we have nodes with inconsistent type.
>> 
>> Summary: CCP disconnects the last path down to root for a data-loop, because it realizes that a `null_check` will never trap. The disconnected state means the types of the data-loop may be left inconsistent.
>> 
>> Right after PhaseCCP, we continue with IGVN.
>> The `LoadB` from that data-part of the loop has `MemNode::Ideal_common` called, which defers its transformation until the type of the address is consistent. However, this is never made consistent, as it is already left inconsistent after PhaseCCP.
>> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/memnode.cpp#L351-L358
>> Note that we only re-push if there is another node in the worklist - a node that hopefully has something to do with the address.
>> But in our case it is just the two LoadB nodes, which were generated from the same `split_through_phi`.
>> 
>> **Solution:**
>> At the end of PhaseCCP, we remove all nodes that were not visited (and may have an inconsistent state). We can do this because we visited all nodes that are still relevant. Rolands fix already made sure that SafePointNodes are visited, such that infinite loops are covered as well.
>> 
>> Regression test added.
>> Test suite passed.
>> Tests rerunning for review suggestion (commit 3)...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   implementing Christians review suggestions

Thanks for doing the updates!

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10250