RFR: 8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop

[Tobias Hartmann]

On Thu, 22 Sep 2022 14:38:33 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> **Context**
> In `IdealLoopTree::do_remove_empty_loop`, we detect empty loops, and break them such that `IGVN` can clean away the loops.
> 
> For this, we used to simply replace the tripcount `Phi` node with the final iv. This has two intended effects:
> 1. The loop-condition collapses to false, and the `CountedLoopEnd` node itself collapses, as the back edge is not taken.
> 2. The Phi may have had other users after the loop, that effectively want to use the loop exit value. Replacing the Phi with the final iv makes sure those values are still correct.
> 
> **Problem**
> We assume that replacing the tripcount necessarily leads to Optimizations of the nodes down to the `CountedLoopEnd`. If this optimization is not done, we are left with a broken loop that IGVN does not clean up. Later optimizations might now find the remnants of the loop, and assume the tripcount `Phi` still exists - but it does not.
> 
> In this concrete example, I had an AddI node that did not do the constant folding. We had
> 
> 100 AddI <x> <y>
> 101 AddI 100 <int:1>
> 
> Now, if `<y>` gets replaced with a constant `<int:2>`, output node `100 AddI` is notified via `PhaseIterGVN::add_users_to_worklist`, however node `101 AddI` is an output of the output - we currently do not notify it. But we would expect that it is folded to:
> 
> 
> 102 AddI <x> <int:3>
> 
> This optimization does not happen, and the propagation down does not take place, prohibiting that the loop condition is determined to be `false` during IGVN.
> 
> **Solution**
> I saw two approaches to fix this issue:
> 
> 1. Ensure that all required optimizations down to the `CountedLoopEnd` are performed. This is difficult as it is easy to miss a small pattern. Here I could simply notify outputs of outputs, if they are both `AddI` nodes. Probably there will eventually be other little Optimizations that were missed, and then we have to fix them all one by one.
> 2. Simplify the required optimization. If we already know that the loop condition is `false`, why not directly replace the condition of the `CountedLoopEnd` with `int:0`? We still need to replace the tripcount `Phi` with the final iv, to ensure exit values are correct for users after the loop. But we can simplify the code in `IdealLoopTree::do_remove_empty_loop` substantially, as we do not need to clone the `cmp` node any more.
> 
> I picked solution 2. I suggest that we still perform solution 1, in an RFE like [JDK-8257197](https://bugs.openjdk.org/browse/JDK-8257197), which catches all optimizations that IGVN could have done but failed to do.
> 
> I added to the regression test.
> 
> - The regression test fails (java assert in test) if I remove the fix belonging to the original regression test:
> https://github.com/openjdk/jdk/commit/4fb2bb554d0d437905ce17b2942e2465cd7f79af#diff-6a59f91cb710d682247df87c75faf602f0ff9f87e2855ead1b80719704fbedffR3132-R3138
> - The regression test fails (dead loop assert) before I apply my fix.
> - Test passes after my fix.
> 
> I ran a large test suite.

Nice analysis, looks good to me too.

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

Marked as reviewed by thartmann (Reviewer).

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