RFR: 8286800: Assert in PhaseIdealLoop::dump_real_LCA is too strong [v3]

[Roberto Castañeda Lozano]

On Fri, 11 Nov 2022 17:33:02 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> We sometimes hit the following assert when dumping a bad graph (before crashing with the bad graph assertion):
>> 
>> assert(real_LCA != NULL, "must always find an LCA"
>> ``` 
>> The algorithm is not correct as we should always find an LCA of two nodes. To fix this, I've re-implemented the algorithm and improved the dumped idom chains:
>> - I limited the node dump to idx + node name to reduce the noise which made it hard to read.
>> - Reversed the idom chain dumps to reflect the graph structure.
>> 
>> Example output:
>> 
>> Bad graph detected in build_loop_late
>> n:  138  CastPP  === 205 38  [[ 263 140 140 168 ]]  #Test:NotNull *  Oop:Test:NotNull * !jvms: Test::mainTest @ bci:40 (line 154)
>> 
>> [... same output as before ...]
>> 
>> idoms of early "197 IfFalse":
>> idom[2]:    42  If
>> idom[1]:    44  IfTrue
>> idom[0]:   196  If
>> n:         197  IfFalse
>> 
>> idoms of (wrong) LCA "205 IfTrue":
>> idom[4]:    42  If
>> idom[3]:    37  Region
>> idom[2]:    73  If
>> idom[1]:    83  IfTrue
>> idom[0]:   204  If
>> n:         205  IfTrue
>> 
>> Real LCA of early "197 IfFalse" (idom[2]) and wrong LCA "205 IfTrue" (idom[4]):
>>   42  If  === 30 41  [[ 43 44 ]] P=0.999000, C=-1.000000 !jvms: Test::mainTest @ bci:32 (line 153)
>> 
>> Tested by manually calling `dump_idoms` during a compilation and by running reproducers of different bad graph assertion bugs.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Change algorithm as suggested by Roberto

Thanks for taking the suggestion into account, looks good!

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

Marked as reviewed by rcastanedalo (Reviewer).

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