8376587: Fatal "dead path discovered by TypeNode during igvn" after C2 compilation

[rwestrel]

A subgraph:

(LoadB _ (Phi (Loop .. ..) .. ..) (AddP _ (DecodeN top) .. ..))

where Loop is reachable but the LoadB is dead (DecodeN's input
is top and the LoadB has no use in a live CFG path), is split thru
phi during IGVN and transformed into:

(Phi (Loop .. ..) (LoadB _ .. (AddP _ (DecodeN top) .. ..)) (LoadB _ .. (AddP _ (DecodeN top) .. ..)))

DecodeN nodes become top next because of it's top
input. TypeNode::Ideal runs (There's no DecodeNNode::Ideal()) and
calls make_paths_from_here_dead() which causes the path on loop
entry to be made dead even though it's reachable.

The make_paths_from_here_dead() was added for cast nodes. The logic
in ConstraintCastNode::Ideal() shows how this is expected to work:

  if (in(1) != nullptr && phase->type(in(1)) != Type::TOP) {
    return TypeNode::Ideal(phase, can_reshape);
  }

TypeNode::Ideal() is only called if the cast becomes top and its
input is not top, that is, it's the type narrowing that the cast
performs that causes the cast to become top not that it's in a dead
subgraph.

For the DecodeN above, there's no type narrowing at the DecodeN so
when the DecodeN becomes top, it shouldn't cause
make_paths_from_here_dead() from being called. I think It was error
prone to have TypeNode::Ideal run make_paths_from_here_dead() for
all nodes that become top so I refactored that code.

I noticed that the igvn split thru phi logic, when it checks whether
inputs dominate the current region, bails out if a dead subgraph is
encountered but the shape above is missed. I fixed that as well.

---

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-8376587: Fatal "dead path discovered by TypeNode during igvn" after C2 compilation (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30339

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back roland! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

Webrevs

• 00: Full (7a5535f6)

[merykitty]

For the DecodeN above, there's no type narrowing at the DecodeN so when the DecodeN becomes top, it shouldn't cause make_paths_from_here_dead() from being called. I think It was error prone to have TypeNode::Ideal run make_paths_from_here_dead() for all nodes that become top so I refactored that code.

I think this is not quite right, if a node becomes top, it propagates that top downwards, no matter the reason it becomes top. As a result, making uses of the top dead seems correct even if there is no type narrowing happening.

I noticed that the igvn split thru phi logic, when it checks whether inputs dominate the current region, bails out if a dead subgraph is encountered but the shape above is missed. I fixed that as well.

I think this is the main culprit. Suppose before the pointer input becomes dead, it is below the Phi, so we should not be able to split the load through that Phi. However, after the pointer input becomes top, it is suddenly dominated by only root, making all_paths_dominate incorrectly declare that we can incorrectly split the load through the Phi.

[merykitty]

I think phase should be a mandatory input of this function, unless we can be sure that we are looking at a canonical graph. There are other uses (such as in MemNode::detect_ptr_independence) that suffer the same issue.

[rwestrel]

Would you say the risk is that MemNode::detect_ptr_independence() returns an incorrect result or a too conservative one?

I initially added the phase input everywhere:
de785ee

but it turned out to be a big change and my understanding was that was only needed for correctness for the split thru phi of a load.

[dafedafe]

It looks like a lot of changes... could a "warning" comment before the method be enough?

[rwestrel]

I noticed that the igvn split thru phi logic, when it checks whether inputs dominate the current region, bails out if a dead subgraph is encountered but the shape above is missed. I fixed that as well.

I think this is the main culprit. Suppose before the pointer input becomes dead, it is below the Phi, so we should not be able to split the load through that Phi. However, after the pointer input becomes top, it is suddenly dominated by only root, making all_paths_dominate incorrectly declare that we can incorrectly split the load through the Phi.

I think you're right. But would you agree that if the DecodeN's input is top then that subgraph is dying and there's no need to propagate top downwards and we should avoid that extra work?

[merykitty]

Would you say the risk is that MemNode::detect_ptr_independence() returns an incorrect result or a too conservative one?

The result of the all_control_dominates is incorrect, although I think it does not matter because the dom input is dying anyway, so the result of detect_ptr_independence is still correct (top is independent with anything). So, maybe it's fine, I just feel it can be dangerous if the function can return an incorrect value when an optional parameter is not present.

I think you're right. But would you agree that if the DecodeN's input is top then that subgraph is dying and there's no need to propagate top downwards and we should avoid that extra work?

Yes, I think for the current implementation of make_paths_from_here_dead, it is reasonable not to do it if the input is dying anyway.

[dafedafe]

Does it still need to be virtual?