8385328: [lworld] C2: assert(phi->_idx >= nodes_size()) failed: only new Phi per instance memory slice still happens after JDK-8374742 by rwestrel · Pull Request #2489 · openjdk/valhalla

rwestrel · 2026-05-29T14:00:42Z

The bug is caused by a mismatch in the way LoadFlat and StoreFlat
are handled in ConnectionGraph::find_inst_mem() and
MemNode::optimize_simple_memory_chain(). Once EA has run a first
time, found some value array allocations as non escaping, and removed
their LoadFlat/StoreFlat, igvn runs and is able to further
transform the memory subgraph which introduces inconsistencies that
are then caught the next time EA runs.

The fix tries to make sure ConnectionGraph::find_inst_mem() and
MemNode::optimize_simple_memory_chain() handle LoadFlat and
StoreFlat similarly.

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8385328: [lworld] C2: assert(phi->_idx >= nodes_size()) failed: only new Phi per instance memory slice still happens after JDK-8374742 (Bug - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Committer) ⚠️ Review applies to 8904c84a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2489

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2489.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-05-29T14:02:39Z

👋 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 · 2026-05-29T14:04:56Z

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

8385328: [lworld] C2: assert(phi->_idx >= nodes_size()) failed: only new Phi per instance memory slice still happens after JDK-8374742

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 16 new commits pushed to the lworld branch:

2cbfd78: 8385928: [lworld] Clean up some redundant core libraries diffs
a7567f2: 8385943: [lworld] Redundant ValueBased annotation on Record
74f27eb: 8385916: [lworld] Cherry-pick JDK-8377808 and remove problemlisting
... and 13 more: https://git.openjdk.org/valhalla/compare/39e74d029fe93cb8c6056b6d21a71bc154b6905a...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

rwestrel · 2026-05-29T14:05:44Z

/template append

openjdk · 2026-05-29T14:09:50Z

@rwestrel The pull request template has been appended to the pull request body

openjdk · 2026-05-29T14:15:25Z

@rwestrel Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2026-05-29T14:19:55Z

Webrevs

dean-long · 2026-05-29T23:55:23Z

+      } else if (proj_in->is_StoreFlat()) {
+        Node* base = proj_in->as_StoreFlat()->base();
+        const TypeOopPtr* t_store_base =igvn->type(base)->is_oopptr();
+        if (t_store_base->instance_id() != toop->instance_id()) {


A comment explaining what is happening here would be helpful. Why does StoreFlat need this check but LoadFlat does not?

I added some comments. Do they look ok to you?

Yes, the comments help, thanks!

TobiHartmann

Looks good to me.

dean-long · 2026-06-01T21:40:04Z

-        }
+        assert(is_strict_final_load || is_known_instance, "tested above");
+        // LoadFlat and StoreFlat cannot happen to strict final fields
+        // LoadFlat and StoreFlat to known instances are removed at the end of EA: this one is unrelated


I asked a particular LLM about this, and it thinks there could be a problem here because a C2_MISMATCHED StoreFlat is not removed, and EA only marks the fields as global escape and not the based object. Do you agree?

A StoreFlat is not a regular Store so it can't be C2_MISMATCHED.

If that's the case then we can add assert to check that in the StoreFlatNode ctor, and remove the check for C2_MISMATCHED at the beginning of StoreFlatNode::expand_non_atomic.

Right. I missed that. It seems C2_MISMATCHED accesses come from unsafe.
Not sure why C2_MISMATCHED would be a problem with StoreFlat but for LoadFlat, I suppose it's safer to not step over a C2_MISMATCHED LoadFlat for the same allocation as alias_idx as that one is not going away.

dean-long · 2026-06-01T21:45:04Z

+        // - or it is a StoreFlat for some object unrelated to alias_idx that can't modify the memory state for alias_idx
+        Node* base = proj_in->as_StoreFlat()->base();
+        const TypeOopPtr* t_store_base =igvn->type(base)->is_oopptr();
+        if (t_store_base->instance_id() != toop->instance_id()) {


Is it impossible to encounter InstanceBot here? This compare is only valid if both sides are known instances, right?

It can nbe InstanceBot on the left hand side. AFAIU, InstanceBot doesn't mean any instance, it means any instance other than the ones that are known. So comparing InstanceBot with a known instance returns false as expected.
This said, one problem with this logic is that it gets types from igvn and they may not have propagated yet. I reworked it to not use the types from igvn.

openjdk · 2026-06-02T11:59:53Z

@rwestrel this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8385328
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

dean-long · 2026-06-03T02:17:25Z

+      return true;
+    }
+  }
+  assert(t_store_base->instance_id() != toop->instance_id(), "should not alias");


I'm not an EA expert, but I'm still not convinced that we can compare instance_id() her directly here without checking for is_known_instance() or unknown edge pointing to phantom_obj. Also, I think such a test would be better as TypeOopPtr helper method. (It is a pet peeve of mine that we expose the implementation detail of how instance IDs and alias IDs are integers.)
Maybe an EA expert like @vnkozlov could check this too.

vnkozlov

Few comments/questions.

vnkozlov · 2026-06-03T15:33:18Z

 //
 #define FIND_INST_MEM_RECURSION_DEPTH_LIMIT 1000
+
+bool ConnectionGraph::flat_access_aliases_with(Node* flat_access, const TypeOopPtr *toop) {


Add comment about what this method does.

vnkozlov · 2026-06-03T16:02:40Z

 #define FIND_INST_MEM_RECURSION_DEPTH_LIMIT 1000
+
+bool ConnectionGraph::flat_access_aliases_with(Node* flat_access, const TypeOopPtr *toop) {
+  Node* base = flat_access->is_StoreFlat() ? flat_access->as_StoreFlat()->base() : flat_access->as_LoadFlat()->base();


What type of base node could be here?

base can be anything that's a pointer (at offset 0) but not an AddP AFAICT

vnkozlov · 2026-06-03T16:12:20Z

+        // - this is a LoadFlat for alias_idx's non escaping allocation: it will get removed by
+        // ConnectionGraph::optimize_flat_accesses() and has no effect on the memory state
+        // - or it is a LoadFlat for some object unrelated to alias_idx
+        // In both cases, it's safe to assume this LoadFlat doesn't modify the memory for alias_idx


Which flat load can modify memory? I surprise that this node has Memory projection.

Whey they are expanded they need memory barriers which is why I think they have a memory projection. If the LoadFlat is from a non escaping object, then it doesn't need memory barriers when it's expanded AFAIU.

vnkozlov · 2026-06-03T16:23:42Z

+        // If the LoadFlat is mismatched, it's not removed so don't step over it if it's a flat access to the alias_idx
+        // known instance
+        if (!proj_in->as_LoadFlat()->is_mismatched() || !flat_access_aliases_with(proj_in, toop)) {


When it can be mismatched? Your whole comment above just confuse me.

What I see is in normal case (not mismatched) we always step over loads regardless memory slice it is on.
In mismatching case we skip if it is on different memory slice. That is it.

It's mismatched when it comes from an unsafe call, for instance, if a flat field is read with unsafe at an offset that's not a constant so we don't really know what field is being read.

vnkozlov · 2026-06-03T16:31:54Z

+  PointsToNode::EscapeState es = ptn->escape_state();
+  if (es >= PointsToNode::GlobalEscape) {
+    return false;
+  }


Why not check here? Asserts will not be needed then.

const TypeOopPtr* t_base = _igvn->type(base)->is_oopptr(); if (t_base->instance_id() == toop->instance_id()) { return true; }

That method should return the same as:

t_base->instance_id() == toop->instance_id()

The problem is that igvn hasn't run so types haven't propagated and t_base has a good chance to be a value that's not up to date and can't be relied on.

So yes, what you suggest would work but if feels safer to me to checks that the types and the result of the method are consistent.

vnkozlov · 2026-06-03T16:32:23Z

+
+bool ConnectionGraph::flat_access_aliases_with(Node* flat_access, const TypeOopPtr *toop) {
+  Node* base = flat_access->is_StoreFlat() ? flat_access->as_StoreFlat()->base() : flat_access->as_LoadFlat()->base();
+  const TypeOopPtr* t_base = _igvn->type(base)->is_oopptr();


This statement is too early - far before uses.

rwestrel · 2026-06-04T15:47:21Z

Few comments/questions.

Thanks for looking at this. I pushed a small update.

rwestrel added 3 commits May 29, 2026 16:13

test & fix

2583875

more

7244312

whitespaces

8904c84

rwestrel force-pushed the JDK-8385328 branch from 7df4a25 to 8904c84 Compare May 29, 2026 14:14

openjdk Bot added the rfr Pull request is ready for review label May 29, 2026

dean-long reviewed May 29, 2026

View reviewed changes

TobiHartmann approved these changes Jun 1, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Jun 1, 2026

review

8f88da8

dean-long reviewed Jun 1, 2026

View reviewed changes

more

1032e0a

openjdk Bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jun 2, 2026

Merge branch 'lworld' into JDK-8385328

863a232

openjdk Bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jun 2, 2026

dean-long reviewed Jun 3, 2026

View reviewed changes

review

418790c

vnkozlov reviewed Jun 3, 2026

View reviewed changes

review

4556620

Conversation

rwestrel commented May 29, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented May 29, 2026

Uh oh!

openjdk Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwestrel commented May 29, 2026

Uh oh!

openjdk Bot commented May 29, 2026

Uh oh!

openjdk Bot commented May 29, 2026

Uh oh!

mlbridge Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk Bot commented Jun 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwestrel Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwestrel commented Jun 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

rwestrel commented May 29, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented May 29, 2026 •

edited

Loading

mlbridge Bot commented May 29, 2026 •

edited

Loading

rwestrel Jun 4, 2026 •

edited

Loading