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

[Emanuel Peter]

On Fri, 16 Sep 2022 08:51:31 GMT, Christian Hagedorn <chagedorn 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.
>
> src/hotspot/share/opto/phaseX.cpp line 1986:
> 
>> 1984: 
>> 1985:   while ( transform_stack.is_nonempty() ) {
>> 1986:     Node *clone = transform_stack.pop();
> 
> Even though it's old code, you could also fix the code style when touching the code:
> Suggestion:
> 
>   while (transform_stack.is_nonempty()) {
>     Node* clone = transform_stack.pop();

yes, makes sense, thanks for catching that

> test/hotspot/jtreg/compiler/ccp/TestRemoveUnreachableCCP.java line 3:
> 
>> 1: /*
>> 2:  * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
>> 3:  *
> 
> Looking at other files, I think this empty line should be removed.

ok, will do that

> test/hotspot/jtreg/compiler/ccp/TestRemoveUnreachableCCP.java line 29:
> 
>> 27:  * @bug 8287217
>> 28:  * @summary CCP must remove nodes that are not traversed, else their type can be inconsistent
>> 29:  * @run main/othervm -Xcomp -Xbatch -XX:CompileCommand=compileOnly,TestRemoveUnreachableCCP::test
> 
> `-Xbatch` can be removed as it is implied by `-Xcomp`. I think we should use `compileonly` instead of `compileOnly`. But it is case insensitive, so it does not really matter.

good point

> test/hotspot/jtreg/compiler/ccp/TestRemoveUnreachableCCP.java line 45:
> 
>> 43: 
>> 44:     public static void main(String[] strArr) {
>> 45:         TestRemoveUnreachableCCP _instance = new TestRemoveUnreachableCCP();
> 
> The instance is unused and can be removed.

thanks for catching that

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

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