Integrated: 8294540: Remove Opaque2Node: it is broken and triggers assert

[Emanuel Peter]

On Fri, 2 Dec 2022 09:58:22 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> As @chhagedorn nicely analyzed, `Opaque2` nodes seem to be broken.
> https://github.com/openjdk/jdk/pull/10306
> 
> **Original idea behing Opaque2**
> The idea was to avoid loop-exit values to have both the un-incremented phi-value and the incremented one. Having both may mean using 2 registers, where 1 register could be enough if we only carry the incremented value, and simply decrement after the loop. Opaque2 was inserted to prevent optimizations that would optimize (Phi + inc - inc) down to (Phi). We had a pattern (Phi + inc -> Opaque2 -> - inc), so the add/sub do not get collapsed. Many years back, that used to be fine, and we would only remove Opaque2 nodes once no IGVN round was run anymore. But now, we take Opaque2 nodes out of the graph and then run IGVN again, which undoes all of the effort.
> 
> **Must remove for now, maybe implement properly later**
> For now we remove it, because they can trigger asserts, and is simply a rotten optimization. We think that this optimization should probably be done at the very end anyway, after the last IGVN round.
> 
> Performance tests indicate that removing it now does not lead to slowdown.
> 
> Filed [JDK-8298019](https://bugs.openjdk.org/browse/JDK-8298019) for future investigation / reimplementation.
> 
> **Analysis of the assert**
> 
> In `PhaseIdealLoop::do_unroll`, we check that the main loop has the same limit in the zero-trip-guard as in the loop-exit. For that, we find the `Opaque1` node `opaq` for the zero-trip-guard limit. We compare its input with the `limit` from the loop-exit. This assert is useful, if we change loop-limits we must ensure they are in sync.
> 
> https://github.com/openjdk/jdk/blob/8c472e481676ed0ef475c4989477d5714880c59e/src/hotspot/share/opto/loopTransform.cpp#L2227-L2228
> 
> Unfortunately, we insert two separate `Opaque2` nodes for the zero-trip-guard and loop-exit in this example.
> Sequence of events:
> - We have 2 loops
> - The second loop is pre-main-post split. The main loop eventually takes the pre-incremented Phi value of the first loop as its loop-limit.
> - The first loop eventually detects that its pre-incremented Phi value is used, and inserts Opaque2 nodes for every use. Unfortunately, the zero-trip-guard `CmpI` and the loop-exit `CmpI` both are direct uses of that pre-incremented Phi (this is very rare, as it seems, usually there are other operations in between, and then it is a single use, that eventually feeds into zero-trip-guard and loop-exit-check).
> 
> ![image](https://user-images.githubusercontent.com/32593061/205263253-b5b811cc-504e-4072-9d10-1bc134964c9d.png)

This pull request has now been integrated.

Changeset: 619b68c5
Author:    Emanuel Peter <epeter at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/619b68c5d1908de335cefd536963cadd57472925
Stats:     188 lines in 11 files changed: 1 ins; 172 del; 15 mod

8294540: Remove Opaque2Node: it is broken and triggers assert

Reviewed-by: chagedorn, kvn

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

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