-
Couldn't load subscription status.
- Fork 6.1k
8370332: C2 SuperWord: SIGSEGV because PhaseIdealLoop::split_thru_phi left dead nodes in loop _body #27955
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
base: master
Are you sure you want to change the base?
8370332: C2 SuperWord: SIGSEGV because PhaseIdealLoop::split_thru_phi left dead nodes in loop _body #27955
Conversation
|
👋 Welcome back epeter! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| for (uint j = 1; j < n->req(); j++) { | ||
| Node* in = n->in(j); | ||
| // Check that in is a phi, and n was its only use. | ||
| if (in->is_Phi() && in->in(0) == region && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work if, say, we're splitting:
(Add (Phi ..) (Phi ..)
With a single Phi as input twice? Doesn't the Phi have 2 uses then (the Add, twice)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point. I'll have to see if I can find a reproducer for that...
Analysis:
split_thru_phican split a node out of the loop, through some loop phi. As a consequence, that node and the phi we split through can become dead. Butsplit_thru_phidid not have any logic to yank the dead node and phi from the_body. If this happens in the same loop-opts-phase as a later SuperWord, and that SuperWord pass somehow accesses that loop_body, then we may find dead nodes, which is not expected.It is not ok that
split_thru_phileaves dead nodes in the_body, so they have to be yanked.What I did additionally: I went through all uses of
split_thru_phi, and moved thereplace_nodefrom the call-site to the method itself. Removing the node and yanking from_bodyconceptually belongs together, so they should be together in code.I suspect that
split_thru_phiwas broken for a long time already. But JDK26 changes in SuperWord started to check inputs of all nodes in_body, and that fails with dead nodes.Future Work:
VerifyLoopOptimizationswork again, we should assert that there are no dead nodes in the_body. We may do that with the following task, or a subsequent one.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27955/head:pull/27955$ git checkout pull/27955Update a local copy of the PR:
$ git checkout pull/27955$ git pull https://git.openjdk.org/jdk.git pull/27955/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27955View PR using the GUI difftool:
$ git pr show -t 27955Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27955.diff
Using Webrev
Link to Webrev Comment