RFR: 8280126: C2: detect and remove dead irreducible loops [v3]

[Christian Hagedorn]

On Wed, 25 Jan 2023 10:58:11 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/cfgnode.cpp line 607:
>> 
>>> 605:     if (is_unreachable_from_root(phase)) {
>>> 606:       // The irreducible loop is dead - must remove it
>>> 607:       PhaseIterGVN* igvn = phase->is_IterGVN();
>> 
>> Since `phase->is_iterGVN()` is used at various places during this method, it might be worth to define the variable once and then reuse it.
>
> We could do that. But all of those cases are guarded by a `if(can_reshape)`. So we only ever ask for `phase->is_IterGVN()` if it actually is `not-null`. That has its own appeal. Maybe we can refactor the function, but I'd rather not make this changeset even more complex.

That's fine to clean it up separately at some point.

>> src/hotspot/share/opto/cfgnode.cpp line 608:
>> 
>>> 606:       // The irreducible loop is dead - must remove it
>>> 607:       PhaseIterGVN* igvn = phase->is_IterGVN();
>>> 608:       remove_unreachable_subgraph(igvn);
>> 
>> Since we are now always eagerly removing unreachable subgraphs in this case and for normal dead loops, I guess we can get rid of the caching of the `is_unreachable_from_root()` result managed with the `_is_unreachable_region` field.
>
> I don't think that this is true. Note that only `is_unreachable_region` does the caching, and assumes that we have a canonical loop (one entry-control, one backedge). It is used both in `RegionNode::Ideal` (twice, so caching makes sense already. Without the caching the logic of that if becomes much more complex) and in `PhiNode::is_data_loop` (a phi might realize the loop is dead before the loop-head does, and already do the traversal - if the result is "unreachable", then we would like to have that value cached and immediately returned for the loop head).
> Do you agree?

I've missed the use in `PhiNode::is_data_loop()` and only looked at the uses in `RegionNode::Ideal()` (we would only set the cache to true when we are also calling `remove_unreachable_subgraph()` right afterwards). So, I agree to leave this in.

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

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