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

[Christian Hagedorn]

On Mon, 18 Mar 2024 17:15:38 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> The assert fails because peeling happens at a single entry
> `Region`. That `Region` only has a single input because other inputs
> were found unreachable and removed by
> `PhaseIdealLoop::Dominators()`. The fix I propose is to have
> `PhaseIdealLoop::Dominators()` remove the `Region` and its `Phi`s
> entirely in this case.

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

test/hotspot/jtreg/compiler/loopopts/TestPartialPeelingAtSingleInputRegion.java line 2:

> 1: /*
> 2:  * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

test/hotspot/jtreg/compiler/loopopts/TestPartialPeelingAtSingleInputRegion.java line 51:

> 49: 
> 50:     public static void main(String[] args) {
> 51:         for (int i = 0; i < 50_000; ++i) {

Just a minor thing: Do you really need 50000 iterations or would fewer be sufficient to trigger the bug?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18353#discussion_r1535828422
PR Review Comment: https://git.openjdk.org/jdk/pull/18353#discussion_r1535822221
PR Review Comment: https://git.openjdk.org/jdk/pull/18353#discussion_r1535823273