RFR: 8257197: Add additional verification code to PhaseCCP

[Christian Hagedorn]

On Tue, 6 Dec 2022 08:02:12 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> **Targetted for JDK-21.**
> 
> We have had many bugs that could be tracked back to missing optimizations during PhaseCCP.
> Often the problem is that a node `x` has an input (of input of input) `y` which is modified, but `y` does not notify `x` (does not push it to the worklist). For one this is a missed opportunity to optimize, but it can also lead to failed assumptions later: often we assume that all that can be optimize is already optimized.
> This verification helps us debug faster, and can also help when `Value` optimizations suddenly do further-reaching traversals, which would require further-reaching notifications.
> 
> Sadly, the verification is not total: we have some exceptions. Especially for `LoadNode`, which perform a walk up the memory inputs, which can go arbitrarily far, to look for relates `StoreNode`s. Notification would thus have to put very many nodes on the worklist. The question is if the potential additional optimization is worth the compile-time. If we decided yes, then one we might want to implement a listener-style notification: when a node visits inputs during `Value`, it could subscribe to all (or the relevant) visited input-nodes for future updates. Currently, we just mostly do fixed 1-hop or 2-hop notification of output nodes.
> 
> FYI: I plan to do a similar verification, and a refactoring of `PhaseCCP::push_child_nodes_to_worklist` and `PhaseIterGVN::add_users_to_worklist` in [JDK-8298094](https://bugs.openjdk.org/browse/JDK-8298094).

This is a nice additional verification! I've tried your patch out and it would have indeed found https://github.com/openjdk/jdk19/pull/65 and https://github.com/openjdk/jdk/pull/11448 (in both bugs we've missed to re-add nodes to the CCP worklist).

It's a simple but powerful verification pass, so I think it is okay to add some exceptions in order to make this as stable as possible to avoid false positives/okay-to-miss optimizations.

src/hotspot/share/opto/phaseX.cpp line 1829:

> 1827:           continue; // ignore long widen
> 1828:         }
> 1829:       }

Could these two cases be merged by using the common super type `TypeInteger`, `lo/hi_as_long()` and `isa_integer(T_INT/T_LONG)`?

src/hotspot/share/opto/phaseX.cpp line 1839:

> 1837:       n->dump_bfs(1, 0, "");
> 1838:       tty->print_cr("Current type:");
> 1839:       told->dump_on(tty);

Just an idea: We could think about adding a small padding like 2 whitespaces before the type dump to better emphasize it. But you can also leave it like that.

src/hotspot/share/opto/phaseX.cpp line 1840:

> 1838:       tty->print_cr("Current type:");
> 1839:       told->dump_on(tty);
> 1840:       tty->print_cr("");

You can directly use:
Suggestion:

      tty->cr();

src/hotspot/share/opto/phaseX.cpp line 1843:

> 1841:       tty->print_cr("Optimized type:");
> 1842:       tnew->dump_on(tty);
> 1843:       tty->print_cr("");

I suggest to add another new line here to better separate in case we report multiple nodes.

src/hotspot/share/opto/phaseX.cpp line 1848:

> 1846:   }
> 1847:   // If you get this assert, check if the node was notified of changes in
> 1848:   // the inputs. See PhaseCCP::push_child_nodes_to_worklist

I suggest to rephrase this to also mention the possibility that we might found another exception that we've missed before and cannot reliably handle like `Load` nodes. This could be something like:

// If we get this assert, check why the reported nodes were not processed again in CCP. 
// We should either make sure that these nodes are properly added back to the CCP worklist
// in PhaseCCP::push_child_nodes_to_worklist() to update their type or add an exception
// in the verification code above if that is not possible for some reason (like Load nodes).

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

Marked as reviewed by chagedorn (Reviewer).

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