RFR: 8294540: Remove Opaque2Node: it is broken and triggers assert [v2]

[Vladimir Kozlov]

On Fri, 2 Dec 2022 13:29:39 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.
>> 
>> **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)
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review suggestions from Christian

I agree with removal of `Opaque2`.

On side note, please file RFE to rename all `Opaque` nodes to something meaningful.
With removal of `Opaque2` numbering is broken. Originally it was only 2 such nodes but now we have 4 and it become hassle to find what they do and why we have so many.

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

Marked as reviewed by kvn (Reviewer).

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