8357381: C2: assert(false) failed: should not be here by sarannat · Pull Request #31037 · openjdk/jdk

sarannat · 2026-05-05T10:28:28Z

Issue
The assertion assert(false, "should not be here") in ConnectionGraph::move_inst_mem fires because a StoreB node has StrEquals as a memory user.

Analysis
The main issues starts after the first iteration of iterative escape analysis. As mentioned in JDK-8357381, EA#0 does lock removal. During IGVN after EA#0, the string intrinsic node StrEqual undergoes an ideal transformation (StrIntrinsicNode::Ideal()) which peels its MergeMem input node. At this point StoreB becomes a direct input of StrEquals.

Now EA#1 runs on the transformed graph. Phase 2 of split_unique_types explicitly allows StrEquals as a user of StoreB. In Phase 4, split_unique_types then calls move_inst_mem which is responsible for moving memory users to their memory slices. However, move_inst_mem only handles MergeMem, MemBar, Mem, and Phi as memory user. Since, StrEquals is not handled in move_inst_mem even though it is allowed in Phase 2 of split_unique_types it fires the assertion assert(false, "should not be here").

Solution
In the proposed fix, I have extended move_inst_mem to move string intrinsic nodes that are explicitly allowed in Phase 2 of split_unique_type to their appropriate memory slices.

Testing
Tier1-3 and Github actions

Question to reviewers
Please let me know if this fix looks reasonable.

Thank you @danielogh for the initial analysis.

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 (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8357381: C2: assert(false) failed: should not be here (Bug - P3)(⚠️ The fixVersion in this issue is [28] but the fixVersion in .jcheck/conf is 27, a new backport will be created when this pr is integrated.)

Reviewers

Anton Seoane Ampudia (@anton-seoane - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 31037

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-05-05T10:29:59Z

👋 Welcome back snatarajan! 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 · 2026-05-05T10:30:09Z

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

openjdk · 2026-05-05T10:31:26Z

@sarannat 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.

openjdk · 2026-05-05T10:31:28Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

mlbridge · 2026-05-05T18:00:29Z

Webrevs

vnkozlov · 2026-05-05T18:14:47Z

+    } else if (use->Opcode() == Op_AryEq || use->Opcode() == Op_StrComp || use->Opcode() == Op_CountPositives ||
+          use->Opcode() == Op_StrEquals || use->Opcode() == Op_StrIndexOf || use->Opcode() == Op_StrIndexOfChar) {


Can you add a new method to combine these check into one place and use it here and other places where which check these intrinsics?

anton-seoane · 2026-05-06T07:58:22Z

+      if (alias_idx == general_idx)
+        continue;


Could you add braces here?

anton-seoane · 2026-05-06T08:00:25Z

+        uint orig_uniq = C->unique();
+        Node* m = find_inst_mem(n, general_idx, orig_phis);
+        assert(orig_uniq == C->unique(), "no new nodes");
+        igvn->hash_delete(use);
+        imax -= use->replace_edge(n, m, igvn);
+        igvn->hash_insert(use);
+        record_for_optimizer(use);
+        --i;


Also this pattern (or part of it) seems quite repeated across this if...else. Maybe it would it be worth extracting it?

anton-seoane · 2026-05-06T08:01:20Z

And... I think adding a regression test for this would be easy 😉, and probably worth it

anton-seoane · 2026-05-08T10:00:05Z

  return result;
 }

+Node* ConnectionGraph::find_inst_mem_assert_no_new_node(Node *orig_mem, int alias_idx, GrowableArray<PhiNode *>  &orig_phis) {


Suggested change

Node* ConnectionGraph::find_inst_mem_assert_no_new_node(Node *orig_mem, int alias_idx, GrowableArray<PhiNode *> &orig_phis) {

Node* ConnectionGraph::find_inst_mem_assert_no_new_node(Node* orig_mem, int alias_idx, GrowableArray<PhiNode*>& orig_phis) {

anton-seoane · 2026-05-08T10:00:35Z


  void  move_inst_mem(Node* n, GrowableArray<PhiNode *>  &orig_phis);
  Node* find_inst_mem(Node* mem, int alias_idx,GrowableArray<PhiNode *>  &orig_phi_worklist, uint rec_depth = 0);
+  Node* find_inst_mem_assert_no_new_node(Node* mem, int alias_idx,GrowableArray<PhiNode *>  &orig_phi_worklist);


Suggested change

Node* find_inst_mem_assert_no_new_node(Node* mem, int alias_idx,GrowableArray<PhiNode *> &orig_phi_worklist);

Node* find_inst_mem_assert_no_new_node(Node* mem, int alias_idx,GrowableArray<PhiNode*>& orig_phi_worklist);

sarannat · 2026-05-11T13:39:54Z

Added the test from JDK-8384257 (which is a duplicate) to the regression test as suggested by @chhagedorn.

vnkozlov · 2026-05-11T14:33:00Z

+ * @test
+ * @bug 8357381
+ * @summary C2 compilation fails with C2: assert(false) failed: should not be here
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -Xbatch -XX:CompileCommand=compileonly,compiler.escapeAnalysis.TestReadOnlyStringIntrinsicDuringEA::main


Why you need -XX:+UnlockDiagnosticVMOptions ?

You also need @requires vm.compiler2.enabled I think.

Compile only main will produce only OSR compilation. Is 10K iterations is enough for that?
Consider using -XX:CompileThreshold=n to reduce number of iterations to trigger C2 compilation early.

Thank you for the review. I removed -XX:+UnlockDiagnosticVMOptions and made modifications to address your comments.

vnkozlov · 2026-05-12T16:32:29Z

@sarannat please see GHA failures:

/home/runner/work/jdk/jdk/test/hotspot/jtreg/compiler/escapeAnalysis/TestReadOnlyStringIntrinsicDuringEA.java:37: error: package c159 does not exist
 public class TestReadOnlyStringIntrinsicDuringEA extends c159.HelperBase {
                                                              ^
1 error

anton-seoane · 2026-05-13T07:52:15Z

+    }
+
+    public static void main(String[] strArr) {
+        for (int t = 0; t < 100_000; t++) {


Do we also need 100000 iterations now that CompileThreshold is in place?

anton-seoane

I think this looks good now

merykitty · 2026-05-15T11:40:52Z

-              op == Op_AryEq || op == Op_StrComp || op == Op_CountPositives ||
-              op == Op_StrCompressedCopy || op == Op_StrInflatedCopy || op == Op_VectorizedHashCode ||
-              op == Op_StrEquals || op == Op_StrIndexOf || op == Op_StrIndexOfChar)) {
+              is_mem_read_only_string_intrinsic(use) || op == Op_VectorizedHashCode ||


What about EncodeISOArray? Should it receive similar treatment?

merykitty · 2026-05-15T11:41:41Z

+  return result;
+}
+
+bool ConnectionGraph::is_mem_read_only_string_intrinsic(Node* n) {


This name is a little misleading, AryEq is not a string intrinsic, for example, and why is VectorizedHashCode not here?

openjdk · 2026-06-04T16:30:29Z

@sarannat this pull request can not be integrated into master 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-8357381
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

eme64 · 2026-06-08T14:05:07Z

@sarannat Are you planning to keep working on this? I'd assume not?

@merykitty @vnkozlov It would be a shame if this patch was dropped. We should see that someone picks it up and brings it over the finish line, with proper attribution.

sarannat · 2026-06-08T14:09:47Z

@eme64 : Thank you for the reminder. I do plan to see this PR to completion. I will start addressing the comments and merge-conflicts this week.

bridgekeeper · 2026-07-07T01:38:49Z

@sarannat This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2026-08-04T06:51:36Z

@sarannat This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

eme64 · 2026-08-04T06:54:22Z

@sarannat Are you still interested in working on this?

TobiHartmann · 2026-08-20T11:54:09Z

Reopened with #32467.

intial fix

0590a55

openjdk Bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 5, 2026

sarannat marked this pull request as ready for review May 5, 2026 17:54

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

vnkozlov reviewed May 5, 2026

View reviewed changes

anton-seoane reviewed May 6, 2026

View reviewed changes

sarannat added 2 commits May 8, 2026 09:12

review comments and test

3213157

review comment braces

0501880

anton-seoane reviewed May 8, 2026

View reviewed changes

sarannat added 3 commits May 8, 2026 12:21

review NIT changes

3440a78

review nit fix

957e74d

adding new test from JDK-8384257

ed88d2d

vnkozlov reviewed May 11, 2026

View reviewed changes

fixing test based on comment

33ee9c7

anton-seoane reviewed May 13, 2026

View reviewed changes

testcase fixes and nit changes

de49729

anton-seoane approved these changes May 15, 2026

View reviewed changes

merykitty reviewed May 15, 2026

View reviewed changes

openjdk Bot added the merge-conflict Pull request has merge conflict with target branch label Jun 4, 2026

bridgekeeper Bot closed this Aug 4, 2026

TobiHartmann mentioned this pull request Aug 20, 2026

8357381: C2: assert(false) failed: should not be here #32467

Closed

4 tasks

		} else if (use->Opcode() == Op_AryEq \|\| use->Opcode() == Op_StrComp \|\| use->Opcode() == Op_CountPositives \|\|
		use->Opcode() == Op_StrEquals \|\| use->Opcode() == Op_StrIndexOf \|\| use->Opcode() == Op_StrIndexOfChar) {

	Node* ConnectionGraph::find_inst_mem_assert_no_new_node(Node orig_mem, int alias_idx, GrowableArray<PhiNode > &orig_phis) {
	Node* ConnectionGraph::find_inst_mem_assert_no_new_node(Node* orig_mem, int alias_idx, GrowableArray<PhiNode*>& orig_phis) {

	Node* find_inst_mem_assert_no_new_node(Node* mem, int alias_idx,GrowableArray<PhiNode *> &orig_phi_worklist);
	Node* find_inst_mem_assert_no_new_node(Node* mem, int alias_idx,GrowableArray<PhiNode*>& orig_phi_worklist);

Uh oh!

Conversation

sarannat commented May 5, 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 5, 2026

Uh oh!

openjdk Bot commented May 5, 2026

Uh oh!

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

Uh oh!

Uh oh!

openjdk Bot commented May 5, 2026

Uh oh!

mlbridge Bot commented May 5, 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!

anton-seoane commented May 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarannat commented May 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anton-seoane 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!

openjdk Bot commented Jun 4, 2026

Uh oh!

eme64 commented Jun 8, 2026

Uh oh!

sarannat commented Jun 8, 2026

Uh oh!

bridgekeeper Bot commented Jul 7, 2026

Uh oh!

bridgekeeper Bot commented Aug 4, 2026

Uh oh!

eme64 commented Aug 4, 2026

Uh oh!

TobiHartmann commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

sarannat commented May 5, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented May 5, 2026 •

edited

Loading

mlbridge Bot commented May 5, 2026 •

edited

Loading