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

[Christian Hagedorn]

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.
> 
> **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)

Nice summary! I agree with this solution. A workaround for the assert seems to be complicated given that `Opaque2` nodes actually do not really fulfill their original purpose anymore as you've stated and also discussed in https://github.com/openjdk/jdk/pull/10306.

> Must remove for now, maybe implement properly later

Right, we could still come back to this optimization and implement it properly if it turns out to be beneficial.

src/hotspot/share/opto/loopTransform.cpp line 2292:

> 2290:       if (loop_head->unrolled_count() == 1) { // only for first unroll
> 2291:         assert(has_ctrl(opaq), "should have it");
> 2292:       }

Could directly be simplified into `assert(loop_head->unrolled_count() != 1 || has_ctrl(opaq), "should have opaque")`.

src/hotspot/share/opto/opaquenode.hpp line 79:

> 77:   int _opt; // what optimization it was used for
> 78:   virtual uint hash() const;
> 79:   virtual bool cmp(const Node &n ) const;

Suggestion:

  virtual bool cmp(const Node &n) const;

src/hotspot/share/opto/opaquenode.hpp line 81:

> 79: // temp register and an extra move).  If we "accidentally" optimize through
> 80: // this kind of a Node, we'll get slightly pessimal, but correct, code.  Thus
> 81: // it's OK to be slightly sloppy on optimizations here.

Not sure if we should also add this comment again to the `Opaque3` nodes as it suggests that it is okay to be sloppy with `Opaque3` as well (but then on the other hand, what exactly is the definition of being sloppy in the context of `Opaque3` nodes? So, it might be more confusing than actually helping).

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

Marked as reviewed by chagedorn (Reviewer).

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