Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8260363: [lworld] C2 compilation fails with assert(n->Opcode() != Op_Phi) failed: cannot match #317

Closed
wants to merge 1 commit into from

Conversation

rwestrel
Copy link
Collaborator

@rwestrel rwestrel commented Jan 28, 2021

This is caused by a StoreCM whose OopStore doesn't point to a Store
node because it was optimized out but to a memory Phi. The Phi is for
slice TypeAryPtr::INLINES. When
Compile::adjust_flattened_array_access_aliases() runs, the
TypeAryPtr::INLINES slice becomes dead and is replaced by independent
slices for each field of flat array elements. Because the StoreCM
doesn't point to a Store, it's impossible to tell what slice it's for
so its input still points to the TypeAryPtr::INLINES Phi once
Compile::adjust_flattened_array_access_aliases() is over.

Given the TypeAryPtr::INLINES slice is dead that's a problem: assuming
we can end up with a StoreCM that points to a Phi for a non eliminated
Store (the Store would be behind the Phi I suppose in that case), then
after Compile::adjust_flattened_array_access_aliases() runs, the Store
would have moved to a new Phi but the StoreCM wouldn't have been
updated.

But that's not what causes the crash. The crash occurs because the
TypeAryPtr::INLINES memory subgraph is partially killed by
Compile::adjust_flattened_array_access_aliases() so when matching
occurs the Phi is only reachable through a precedence edge from the
StoreCM (final graph reshaping moves the OopStore input to precedence
edges). It's unreachable from root at matching time (because that code
doesn't follow precendence edge) and is not marked as shared
by Matcher::find_shared().

When Compile::adjust_flattened_array_access_aliases() runs, the
StoreCM OopStore is actually first set to a MergeMem node that
captures slices created for flat array fields but when
StoreCMNode::Ideal() executes it sets the input to the
TypeAryPtr::INLINES input of the MergeMem. I think that last steps is
what's incorrect after
Compile::adjust_flattened_array_access_aliases(). So I changed that
logic so it keeps the MergeMem input and also changed final graph
reshape so it adds as many precedence edges as there are flat field
inputs to the MergeMem. These changes are required to preserve
correctness, I believe.

With that, i still hit the assert because the bottom memory input to
the MergeMem can end up as a precedence edge to the StoreCM (if that
entry to the MergeMem is top). That bottom memory in the case of the
regression test is a Phi that was created by
Compile::adjust_flattened_array_access_aliases() and is usually
optimized out. It's only reachable from the StoreCM precedence edge. I
don't think there's a fundamental problem here so I change matching so
it looks at precedence edges when looking for shared node. I also had
to change node scheduling in a similar way.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8260363: [lworld] C2 compilation fails with assert(n->Opcode() != Op_Phi) failed: cannot match

Reviewers

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/317/head:pull/317
$ git checkout pull/317

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 28, 2021

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

@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@rwestrel 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:

8260363: [lworld] C2 compilation fails with assert(n->Opcode() != Op_Phi) failed: cannot match

Reviewed-by: thartmann

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 no new commits pushed to the lworld branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks scary but reasonable to me! :)

@rwestrel
Copy link
Collaborator Author

@rwestrel rwestrel commented Feb 3, 2021

Looks scary but reasonable to me! :)

right! Thanks for the review.

@rwestrel
Copy link
Collaborator Author

@rwestrel rwestrel commented Feb 3, 2021

/integrate

@openjdk openjdk bot closed this Feb 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 3, 2021

@rwestrel Since your change was applied there have been 610 commits pushed to the lworld branch:

  • 8569088: Merge jdk
  • bd81ccf: 8259957: Build failure without C1 Compiler after JDK-8258004
  • dfee7b8: 8259511: java/awt/Window/MainKeyWindowTest/TestMainKeyWindow.java failed with "RuntimeException: Test failed: 20 failure(s)"
  • 14ce8f1: 8259870: zBarrier.inline.hpp should not include javaClasses.hpp
  • a1a851b: Merge
  • bb0821e: 8258643: [TESTBUG] javax/swing/JComponent/7154030/bug7154030.java failed with "Exception: Failed to hide opaque button"
  • cd25bf2: 8259574: SIGSEGV in BFSClosure::closure_impl
  • d5ca3b3: 8259641: C2: assert(early->dominates(LCA)) failed: early is high enough
  • 492bebc: 8258004: Remove unnecessary inclusion of vm_version.hpp
  • 533a2d3: 8258961: move some fields of SafePointNode from public to protected
  • ... and 600 more: https://git.openjdk.java.net/valhalla/compare/81e6a5af488474f84674080d0f09c3813b8deb2e...lworld

Your commit was automatically rebased without conflicts.

Pushed as commit fbb8a6b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants