8294540: Remove Opaque2Node: it is broken and triggers assert #11477

eme64 · 2022-12-02T09:58:22Z

As @chhagedorn nicely analyzed, Opaque2 nodes seem to be broken.
#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 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.

jdk/src/hotspot/share/opto/loopTransform.cpp

Lines 2227 to 2228 in 8c472e4

    
           // Zero-trip test uses an 'opaque' node which is not shared. 
        
           assert(opaq->outcnt() == 1 && opaq->in(1) == limit, "");

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8294540: Remove Opaque2Node: it is broken and triggers assert

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11477/head:pull/11477
$ git checkout pull/11477

Update a local copy of the PR:
$ git checkout pull/11477
$ git pull https://git.openjdk.org/jdk pull/11477/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11477

View PR using the GUI difftool:
$ git pr show -t 11477

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11477.diff

bridgekeeper · 2022-12-02T10:00:21Z

👋 Welcome back epeter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-12-02T10:04:15Z

@eme64 The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-12-02T10:07:26Z

Webrevs

01: Full - Incremental (17954728)
00: Full (36e2180c)

chhagedorn

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 #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.

chhagedorn · 2022-12-02T11:39:33Z

src/hotspot/share/opto/opaquenode.hpp

+class Opaque3Node : public Node {
+  int _opt; // what optimization it was used for
+  virtual uint hash() const;
+  virtual bool cmp(const Node &n ) const;


Suggested change

virtual bool cmp(const Node &n ) const;

virtual bool cmp(const Node &n) const;

will do that 👍

chhagedorn · 2022-12-02T12:13:29Z

src/hotspot/share/opto/opaquenode.hpp

-// this kind of a Node, we'll get slightly pessimal, but correct, code.  Thus
-// 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).

I would leave it out, unless if we are sure that it really applies to Opaque3 Nodes.

chhagedorn · 2022-12-02T12:37:42Z

src/hotspot/share/opto/loopTransform.cpp

      if (loop_head->unrolled_count() == 1) { // only for first unroll
-        // Separate limit by Opaque node in case it is an incremented
-        // variable from previous loop to avoid using pre-incremented
-        // value which could increase register pressure.
-        // Otherwise reorg_offsets() optimization will create a separate
-        // Opaque node for each use of trip-counter and as result
-        // zero trip guard limit will be different from loop limit.
        assert(has_ctrl(opaq), "should have it");
-        Node* opaq_ctrl = get_ctrl(opaq);
-        limit = new Opaque2Node(C, limit);
-        register_new_node(limit, opaq_ctrl);
      }


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

will do that 👍

openjdk · 2022-12-02T12:54:07Z

@eme64 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8294540: Remove Opaque2Node: it is broken and triggers assert

Reviewed-by: chagedorn, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 27 new commits pushed to the master branch:

82561de: 8296384: [TESTBUG] sun/security/provider/SecureRandom/AbstractDrbg/SpecTest.java intermittently timeout
61b7093: 8297872: Non-local G1MonotonicArenaFreePool::_freelist_pool has non-trivial ctor/dtor
3b3bbe5: 8296907: VMError: add optional callstacks, siginfo for secondary errors
a573923: 8297264: C2: Cast node is not processed again in CCP and keeps a wrong too narrow type which is later replaced by top
b49fd92: 8298055: AArch64: fastdebug build fails after JDK-8247645
914ef07: 8297609: Add application/wasm MIME type for wasm file extension
a71d91b: 8298067: Persistent test failures after 8296012
87572d4: 8298068: ProblemList tests failing due to JDK-8297235
0edb5d0: 8297683: Use enhanced-for cycle instead of Enumeration in java.security.jgss
c67166f: 8298003: NMT: fatal block printout does not show the correct corruption address
... and 17 more: https://git.openjdk.org/jdk/compare/687fd714bbc390f486272e05452f038bc3631be1...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

vnkozlov

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.

chhagedorn · 2022-12-03T01:05:22Z

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.

That is a good idea. I've planned to rename Opaque4 nodes and get rid of some of their usages in the redesign of the skeleton predicates (JDK-8288981). But it could already be done earlier together with renaming the other opaque nodes now that we get rid of Opaque2 nodes.

chhagedorn

Update looks good!

eme64 · 2022-12-05T08:28:20Z

Thanks @chhagedorn for the previous investigation into Opaque2, and the review! Thanks @vnkozlov for the review!
I created two RFE, one for re-investigation/re-implementation of Opaque2, and one for renaming of Opaque nodes in general.
/integrate

openjdk · 2022-12-05T08:30:49Z

Going to push as commit 619b68c.
Since your change was applied there have been 27 commits pushed to the master branch:

82561de: 8296384: [TESTBUG] sun/security/provider/SecureRandom/AbstractDrbg/SpecTest.java intermittently timeout
61b7093: 8297872: Non-local G1MonotonicArenaFreePool::_freelist_pool has non-trivial ctor/dtor
3b3bbe5: 8296907: VMError: add optional callstacks, siginfo for secondary errors
a573923: 8297264: C2: Cast node is not processed again in CCP and keeps a wrong too narrow type which is later replaced by top
b49fd92: 8298055: AArch64: fastdebug build fails after JDK-8247645
914ef07: 8297609: Add application/wasm MIME type for wasm file extension
a71d91b: 8298067: Persistent test failures after 8296012
87572d4: 8298068: ProblemList tests failing due to JDK-8297235
0edb5d0: 8297683: Use enhanced-for cycle instead of Enumeration in java.security.jgss
c67166f: 8298003: NMT: fatal block printout does not show the correct corruption address
... and 17 more: https://git.openjdk.org/jdk/compare/687fd714bbc390f486272e05452f038bc3631be1...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-05T08:31:08Z

@eme64 Pushed as commit 619b68c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8294540: Remove Opaque2Node: it is broken and triggers asserts

36e2180

eme64 changed the title ~~8294540: Remove Opaque2Node: it is broken and triggers asserts~~ 8294540: Remove Opaque2Node: it is broken and triggers assert Dec 2, 2022

eme64 marked this pull request as ready for review December 2, 2022 09:59

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 2, 2022

chhagedorn approved these changes Dec 2, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 2, 2022

Review suggestions from Christian

1795472

vnkozlov approved these changes Dec 2, 2022

View reviewed changes

chhagedorn approved these changes Dec 5, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 5, 2022

openjdk bot closed this Dec 5, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8294540: Remove Opaque2Node: it is broken and triggers assert #11477

8294540: Remove Opaque2Node: it is broken and triggers assert #11477

eme64 commented Dec 2, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 2, 2022

openjdk bot commented Dec 2, 2022

mlbridge bot commented Dec 2, 2022 •

edited

Loading

chhagedorn left a comment

chhagedorn Dec 2, 2022

eme64 Dec 2, 2022

chhagedorn Dec 2, 2022

eme64 Dec 2, 2022

chhagedorn Dec 2, 2022

eme64 Dec 2, 2022

openjdk bot commented Dec 2, 2022 •

edited

Loading

vnkozlov left a comment

chhagedorn commented Dec 3, 2022

chhagedorn left a comment

eme64 commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

	// Zero-trip test uses an 'opaque' node which is not shared.
	assert(opaq->outcnt() == 1 && opaq->in(1) == limit, "");

	virtual bool cmp(const Node &n ) const;
	virtual bool cmp(const Node &n) const;

		// this kind of a Node, we'll get slightly pessimal, but correct, code. Thus
		// it's OK to be slightly sloppy on optimizations here.

8294540: Remove Opaque2Node: it is broken and triggers assert #11477

8294540: Remove Opaque2Node: it is broken and triggers assert #11477

Conversation

eme64 commented Dec 2, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 2, 2022

openjdk bot commented Dec 2, 2022

mlbridge bot commented Dec 2, 2022 • edited Loading

Webrevs

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Dec 2, 2022

Choose a reason for hiding this comment

eme64 Dec 2, 2022

Choose a reason for hiding this comment

chhagedorn Dec 2, 2022

Choose a reason for hiding this comment

eme64 Dec 2, 2022

Choose a reason for hiding this comment

chhagedorn Dec 2, 2022

Choose a reason for hiding this comment

eme64 Dec 2, 2022

Choose a reason for hiding this comment

openjdk bot commented Dec 2, 2022 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

chhagedorn commented Dec 3, 2022

chhagedorn left a comment

Choose a reason for hiding this comment

eme64 commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

openjdk bot commented Dec 5, 2022

eme64 commented Dec 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 2, 2022 •

edited

Loading

openjdk bot commented Dec 2, 2022 •

edited

Loading