RFR: 8321278: C2: Partial peeling fails with assert "last_peel <- first_not_peeled" [v2]

[Roland Westrelin]

On Fri, 22 Mar 2024 16:06:00 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update test/hotspot/jtreg/compiler/loopopts/TestPartialPeelingAtSingleInputRegion.java
>>   
>>   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>
> src/hotspot/share/opto/domgraph.cpp line 512:
> 
>> 510:         remove_single_entry_region(t, tdom, dom, _igvn);
>> 511:       }
>> 512:       _idom[t->_control->_idx] = dom; // Set immediate dominator
> 
> Removing the regions during `Dominators()` seems reasonable. I guess doing a pass of IGVN after `Dominators()` is probably too much to get these regions removed?
> 
> Could you also remove the region and the phi where the unreachable loops are cleaned up and the region and phis become single-entry nodes? I.e. here:
> https://github.com/openjdk/jdk/blob/ce7ebaa606f96fdfee66d300b56022d9903b5ae3/src/hotspot/share/opto/domgraph.cpp#L453-L463

Thanks for reviewing this.

> Removing the regions during `Dominators()` seems reasonable. I guess doing a pass of IGVN after `Dominators()` is probably too much to get these regions removed?

It does feel like a lot of overhead for such a simple corner case.

> Could you also remove the region and the phi where the unreachable loops are cleaned up and the region and phis become single-entry nodes? I.e. here:

I considered it but it felt harder. The algorithm collects cfg nodes in dfs and then iterate over them several times. If we remove a region at the point you mention, it would need to be removed from the dfs node list. Or we would need to make later iterations over the dfs node list handle dead region nodes. It's not a problem when it's done later as I propose here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18353#discussion_r1535873143