RFR: 8330853: Add missing checks for ConnectionGraph::can_reduce_cmp() call [v2]

[Igor Veresov]

On Tue, 23 Apr 2024 16:14:38 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> In Leyden testing CI we start hitting assert:
>> 
>> 
>> # Internal Error (/workspace/open/src/hotspot/share/opto/node.hpp:407), pid=3216007, tid=3216030
>> # assert(i < _max) failed: oob: i=2, _max=2
>> 
>> Stack: [0x00007f20b011b000,0x00007f20b021b000], sp=0x00007f20b02157e0, free space=1001k
>> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
>> V [libjvm.so+0xbf5bb4] Node::in(unsigned int) const [clone .part.0]+0x24 (node.hpp:407)
>> V [libjvm.so+0xbf69ea] ConnectionGraph::can_reduce_cmp(Node*, Node*) const+0x9a (node.hpp:407)
>> V [libjvm.so+0xbf74a6] ConnectionGraph::can_reduce_check_users(Node*, unsigned int) const+0x996 (escape.cpp:578)
>> 
>> 
>> [JDK-8316991](https://bugs.openjdk.org/browse/JDK-8316991) added new code which assumes that we always have sequence : IfNode->Bool->Cmp: [escape.cpp#L569](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/escape.cpp#L569)
>> which is not true in some cases. In failed cases there is additional Opaque4 node between If and Bool nodes.
>> 
>> The fix is to add missing checks for If, Bool and Cmp nodes.
>> 
>> The fix is verified in Leyden CI testing. The failure happens only with special C2 mode compilation in Leyden.
>> Running our regular tier1-3,stress,xcomp too.
>
> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove redundant opcode check

src/hotspot/share/opto/escape.cpp line 576:

> 574:             if ((opc == Op_CmpP || opc == Op_CmpN) && !can_reduce_cmp(n, iff_cmp)) {
> 575:               NOT_PRODUCT(if (TraceReduceAllocationMerges) tty->print_cr("Can NOT reduce Phi %d on invocation %d. CastPP %d doesn't have simple control.", n->_idx, _invocation, use->_idx);)
> 576:               NOT_PRODUCT(n->dump(5);)

It's a preexisting problem but do you want to call `dump()` unconditionally? Shouldn't it be under `if (TraceReduceAllocationMerges)` ?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18916#discussion_r1578291680