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

Emanuel Peter epeter at openjdk.org
Fri Dec 2 13:29:39 UTC 2022


> 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

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/11477/files
  - new: https://git.openjdk.org/jdk/pull/11477/files/36e2180c..17954728

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11477&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11477&range=00-01

  Stats: 5 lines in 3 files changed: 0 ins; 2 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/11477.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11477/head:pull/11477

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


More information about the hotspot-dev mailing list