Skip to content
This repository has been archived by the owner. It is now read-only.

8269820: C2 PhaseIdealLoop::do_unroll get wrong opaque node #255

Closed
wants to merge 1 commit into from

Conversation

@huishi-hs
Copy link

@huishi-hs huishi-hs commented Jul 19, 2021

More discussion in previous PR(#208) is confilict with fix for JDK-8269752.

Opaque1 in main loop entry is expected only used in compare node's second input. However split if might clone opaque1 and replace original node with phi of opaque1 node. This causes assertion in CountedLoopNode::is_canonical_loop_entry now.

Fix is in BoolNode::Ideal, avoiding switching compare node's input when second input is phi of opaque1.

Test: Linux X64 tier1/2/3 release/fastdebug no regression.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8269820: C2 PhaseIdealLoop::do_unroll get wrong opaque node

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/255/head:pull/255
$ git checkout pull/255

Update a local copy of the PR:
$ git checkout pull/255
$ git pull https://git.openjdk.java.net/jdk17 pull/255/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 255

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/255.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 19, 2021

👋 Welcome back hshi! 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 openjdk bot added the rfr label Jul 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 19, 2021

@huishi-hs The following label will be automatically applied to this pull request:

  • hotspot-compiler

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
Copy link

@mlbridge mlbridge bot commented Jul 19, 2021

Webrevs

@rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Jul 20, 2021

The proposed fix is ok but I wonder how robust it is. Are we sure there can't be 2 (or more) split thru phi in a row and that the Opaque1 node is not hiding behind a chain of Phis?

Note that the bug occurs because the Opaque1 node has control above the predicates due to this:
https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356
That logic is not required for Opaque1 nodes AFAICT. So maybe a simpler fix would be to skip it for Opaque1 nodes.

@huishi-hs
Copy link
Author

@huishi-hs huishi-hs commented Jul 25, 2021

@rwestrel Thanks for you review!

The proposed fix is ok but I wonder how robust it is. Are we sure there can't be 2 (or more) split thru phi in a row and that the Opaque1 node is not hiding behind a chain of Phis?

In normal case, opaque1 has only 1 input, it cannot split more than one time. In special case, opaque1 has two limit inputs, extra one is original loop limit for range check. But this step(IdealLoopTree::iteration_split) happens after split if, so there will not be cases opaque1 behind a chain of Phis in my own understanding. .

Note that the bug occurs because the Opaque1 node has control above the predicates due to this:
https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356
That logic is not required for Opaque1 nodes AFAICT. So maybe a simpler fix would be to skip it for Opaque1 nodes.

Not understand how https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356 cause this bug.
Are you suggesting skipping split if wit Opaque1 in tree as discussed in ealier PR? Skipping must be a safer and conservertive fix, I can submit a new fix if safer fix is perfer now.

@rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Jul 27, 2021

In normal case, opaque1 has only 1 input, it cannot split more than one time. In special case, opaque1 has two limit inputs, extra one is original loop limit for range check. But this step(IdealLoopTree::iteration_split) happens after split if, so there will not be cases opaque1 behind a chain of Phis in my own understanding. .

It's not that obvious to me. Couldn't split thru phi cause Opaque1 to be split several times thru several Regions?

Not understand how https://github.com/openjdk/jdk17/blob/master/src/hotspot/share/opto/loopnode.cpp#L5356 cause this bug.
Are you suggesting skipping split if wit Opaque1 in tree as discussed in ealier PR? Skipping must be a safer and conservertive fix, I can submit a new fix if safer fix is perfer now.

Here is the fix I suggest:
rwestrel@15f515a

Opaque1 is split thru Phi because its control is set to the Region that dominates the predicates. The code I pointed to causes the control to be set that way. But I don't think that logic applies to Opaque1 nodes.

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jul 29, 2021

There could be a case when Opaque1 is used by predicate which follows a Region without been placed there by code Roland is pointing. Then Roland's suggested fix may not work in general case.
And I understand the concern about hidden Opaque1 behind Phis. I need to look on this case to make suggestion. In general we do split_if if we can fold values on one of paths. I think we should not do it for Cmp which reference Opaque1. But I need more details.

I will look on this case today.

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Aug 3, 2021

The issue is more complicated and I need more time to investigate it.
I moved bug Fix version to JDK 18 (latest JDK).
This is NOT new regression in JDK 17 - it failed in previous releases.
Ffailure changed after 8269752 but on the same code path.

Please, keep current PR until we get the fix.

@huishi-hs
Copy link
Author

@huishi-hs huishi-hs commented Aug 7, 2021

@vnkozlov @rwestrel Thanks for your help!

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 10, 2021

The jdk17 repo is no longer open for any fixes. If this PR is to move forward, it will need to be redone as a PR for the mainline openjdk/jdk repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants