8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed

[antonvoznia]

Hi,

I prepare (reopened) a pretty big PR with several backports.
The original fix that is needed 8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed
This fixes EA issues that cause NPE when accessing to an initialized field.

However, this fix introduces a crash that is solved in
8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN
Technically, I need these 2 fixes only. But there are dependencies in the code modifications that I had to pull in with additional backports.

I'm not sure how to bring these 2 fixes into JDK 25 more easily.
I decided to create a chain of backports.
Feel free to suggest any ideas on how I could simplify it.

Testing.

• Locally run on mac arm:

test/hotspot/jtreg/compiler/c2/
test/hotspot/jtreg/compiler/escapeAnalysis/
test/hotspot/jtreg/compiler/igvn/

• GHA Sanity Checks

No regression found.

Below I explain the chain dependencies.

1. 8360031: C2 compilation asserts in MemBarNode::remove
   • Is prerequisite for the following fix, it rewrites MemBarNode::remove, without it the 2. fix crashes on the assert
2. 8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed
   • The fix itself, but introduces a crash in find_inst_mem(). The crash is fixed under 7.
3. 8347273: C2: VerifyIterativeGVN for Ideal and Identity
   • Prerequisite for next fixes to apply them cleanly, for 4. and 7. escpecially.
4. 8351889: C2 crash: assertion failed: Base pointers must match (addp 344)
   • Crash fix and prerequisite since it modifies the common function verify_optimize (entangled with 3.)
5. 8361144: Strenghten the Ideal Verification in PhaseIterGVN::verify_Ideal_for by comparing the hash of a node before and after Ideal
   • Prerequisite for 6., it add old_hash that is modified in 6.
6. 8371536: C2: VerifyIterativeGVN should assert on first detected failure
   • Prerequisite for 7., converts functions verify_*_for from bool to void, this logic is used in the following fix 7.
7. 8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN
   • Fixes crash in find_inst_mem() that was introduced in 1.

Not clean backports:
8371536: C2: VerifyIterativeGVN should assert on first detected failure
- current commit modifies a function that was also changed in 8371581: C2: PhaseCCP should reach fixpoint by revisiting deeply-Value-d nodes
I've not done the backport to minimize the commits chain, so I just extended the function on the strict argument within this commit

8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN
Conflict files:

• both modified: src/hotspot/share/opto/compile.cpp

<<<<<<< HEAD
   igvn.reset_from_gvn(initial_gvn());
   igvn.optimize();
=======
   igvn.reset();
   igvn.optimize(true);
>>>>>>> 3f6271b2b91 (8375442: C2

• both modified: src/hotspot/share/opto/loopnode.cpp

<<<<<<< HEAD
        _igvn.remove_dead_node(clone);
=======
        igvn->remove_dead_node(clone, PhaseIterGVN::NodeOrigin::Speculative);
>>>>>>> 3f6271b2b91 (8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN)

• both modified: src/hotspot/share/opto/loopopts.cpp

<<<<<<< HEAD
  if (wins <= policy) {
    _igvn.remove_dead_node(phi);
=======
  if (!wins.profitable(policy)) {
    _igvn.remove_dead_node(phi, PhaseIterGVN::NodeOrigin::Speculative);
>>>>>>> 3f6271b2b91 (8375442: C2: Notify nodes that inspect the graph deeply of changes far away during 

• both modified: src/hotspot/share/opto/split_if.cpp (I keep current change, but replaced one line with

_igvn.remove_dead_node(region, PhaseIterGVN::NodeOrigin::Graph);)
    <<<<<<< HEAD
  _igvn.remove_dead_node(region);
  if (iff->Opcode() == Op_RangeCheck) {
    // Pin array access nodes: control is updated here to a region. If, after some transformations, only one path
    // into the region is left, an array load could become dependent on a condition that's not a range check for
    // that access. If that condition is replaced by an identical dominating one, then an unpinned load would risk
    // floating above its range check.
    pin_array_access_nodes_dependent_on(new_true);
    pin_array_access_nodes_dependent_on(new_false);
  }
=======
  _igvn.remove_dead_node(region, PhaseIterGVN::NodeOrigin::Graph);

  // Control is updated here to a region, which is not a test, so any node that
  // depends_only_on_test must be pinned
  pin_nodes_dependent_on(new_true, iff->Opcode() == Op_RangeCheck);
  pin_nodes_dependent_on(new_false, iff->Opcode() == Op_RangeCheck);
>>>>>>> 3f6271b2b91 (8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN)
        added by them:   test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/TestDeepIGVNRevisit.java

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

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
• JDK-8347273 needs maintainer approval
• JDK-8371536 needs maintainer approval
• JDK-8375442 needs maintainer approval
• JDK-8360031 needs maintainer approval
• JDK-8327963 needs maintainer approval
• JDK-8361144 needs maintainer approval
• JDK-8351889 needs maintainer approval

Issues

• JDK-8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed (Bug - P3)
• JDK-8360031: C2 compilation asserts in MemBarNode::remove (Bug - P3)
• JDK-8347273: C2: VerifyIterativeGVN for Ideal and Identity (Enhancement - P4)
• JDK-8351889: C2 crash: assertion failed: Base pointers must match (addp 344) (Bug - P3)
• JDK-8361144: Strenghten the Ideal Verification in PhaseIterGVN::verify_Ideal_for by comparing the hash of a node before and after Ideal (Enhancement - P4)
• JDK-8371536: C2: VerifyIterativeGVN should assert on first detected failure (Enhancement - P4)
• JDK-8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk25u-dev.git pull/623/head:pull/623
$ git checkout pull/623

Update a local copy of the PR:
$ git checkout pull/623
$ git pull https://git.openjdk.org/jdk25u-dev.git pull/623/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 623

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk25u-dev/pull/623.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back antonvoznia! 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.

[antonvoznia]

/issue add JDK-8360031, JDK-8347273, JDK-8351889, JDK-8361144, JDK-8371536, JDK-8375442

[openjdk]

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

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[openjdk]

@antonvoznia
Adding additional issue to issue list: 8360031: C2 compilation asserts in MemBarNode::remove.

Adding additional issue to issue list: 8347273: C2: VerifyIterativeGVN for Ideal and Identity.

Adding additional issue to issue list: 8351889: C2 crash: assertion failed: Base pointers must match (addp 344).

Adding additional issue to issue list: 8361144: Strenghten the Ideal Verification in PhaseIterGVN::verify_Ideal_for by comparing the hash of a node before and after Ideal.

Adding additional issue to issue list: 8371536: C2: VerifyIterativeGVN should assert on first detected failure.

Adding additional issue to issue list: 8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN.

[mlbridge]

Webrevs

• 00: Full (9e4f0225)

[antonvoznia]

Hi @GoeLin ,

I'd like to ask for your opinion on how to approach the following case.

I created the fairly large PR that contains 7 commits in this jdk25u-dev repository.

The main C2 issue caused by incorrect EA analysis is solved in a single commit.
8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed
I did a backport of the commit since the issues are in JDK25 as well.
That’s a main fix that would like to bring in jdk25.

Unfortunately, the commit introduces a crash that had been solved later, there:
8375442: C2: Notify nodes that inspect the graph deeply of changes far away during IGVN

Technically, we need just these 2 commits.
However, it’s not possible to make a clean backport of the second one because of some intermediate code changes between then that became prerequisites.
I tried to make this PR as minimal as it’s possible.

What do you think, would it be possible to bring the solution within current PR?
If not, how would we solve the problem in JDK25?
8327963: C2: fix construction of memory graph around Initialize node to prevent incorrect execution if allocation is removed

Thanks!