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

[Emanuel Peter]

On Tue, 6 Dec 2022 12:32:30 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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
>
> 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)`?

refactored it

> 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.

will leave it as is. extra new-line should already help with readability

> 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).

Added your text, thanks

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

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