RFR: 8257197: Add additional verification code to PhaseCCP [v3]

[Christian Hagedorn]

On Wed, 7 Dec 2022 17:08:23 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).
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8257197
>  - Review suggestions from Christian
>  - 8257197: Add additional verification code to PhaseCCP

Thanks for doing the updates, looks good!

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

> 1815:     if (told != tnew) {
> 1816:       // Check special cases that are ok
> 1817:       if (told->isa_integer(tnew->basic_type()) ) { // both either int or long

As `isa_integer()` returns a pointer, it might be better to explicitly check with `!= nullptr`.
Suggestion:

      if (told->isa_integer(tnew->basic_type()) != nullptr) { // both either int or long

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

> 1817:       if (told->isa_integer(tnew->basic_type()) ) { // both either int or long
> 1818:         const TypeInteger *t0 = told->is_integer(tnew->basic_type());
> 1819:         const TypeInteger *t1 = tnew->is_integer(tnew->basic_type());

Suggestion:

        const TypeInteger* t0 = told->is_integer(tnew->basic_type());
        const TypeInteger* t1 = tnew->is_integer(tnew->basic_type());

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

Marked as reviewed by chagedorn (Reviewer).

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