8373591: C2: Fix the memory around some intrinsics nodes by merykitty · Pull Request #28789 · openjdk/jdk

merykitty · 2025-12-12T12:43:14Z

Hi,

This is extracted from #28570 , there are 2 issues here:

Some intrinsics nodes advertise incorrect adr_type. For example, AryEqNode reports adr_type being TypeAryPtr::BYTES (it inherits this from StrIntrinsicNode). This is incorrect, however, as it can accept char[] inputs, too. Another case is VectorizedHashCodeNode, which reports its adr_type being TypePtr::BOTTOM, but it actually extracts a memory slice and does not consume the whole memory.
For nodes such as StrInflatedCopyNode, as they consume more than they produce, during scheduling, we need to compute anti-dependencies. This is not the case, so we should fix it by making the nodes kill all the memory they consume. This issue is often not present because these intrinsics are not exposed bare to general usage.

Testing:

tier1-4,hs-precheckin-comp,hs-comp-stress

Please kindly review, thanks a lot.

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-8373591: C2: Fix the memory around some intrinsics nodes (Bug - P3)

Reviewers

Roland Westrelin (@rwestrel - Reviewer) 🔄 Re-review required (review applies to c3503ed9)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28789

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-12-12T12:45:18Z

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

merykitty · 2025-12-12T12:47:00Z

@eme64 I have extracted the fix of memory around intrinsics nodes in the other PR to this PR and added a unit test for the potential issue.

openjdk · 2025-12-12T12:47:10Z

@merykitty This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-12-12T12:47:58Z

@merykitty 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 · 2025-12-12T12:51:39Z

Webrevs

rwestrel · 2025-12-12T14:12:40Z

src/hotspot/share/opto/graphKit.cpp

  Node* res_mem = _gvn.transform(new SCMemProjNode(_gvn.transform(str)));
-  set_memory(res_mem, TypeAryPtr::BYTES);
+  if (adr_type == TypePtr::BOTTOM) {
+    set_all_memory(res_mem);


I'm confused by this. Doesn't StrCompressedCopyNode only write to dst? So the only part of the memory state that it updates is the one for TypeAryPtr::BYTES?

It is because if a node consumes more memory than it produces, we need to compute its anti-dependencies. And since we do not compute anti-dependencies of these nodes, it is safer to make them kill all the memory they consume. What do you think?

Could this be fixed by appending a MemBarCPUOrderNode on the slice of src?

That's a really great idea! I have implemented it.

openjdk · 2025-12-12T16:02:57Z

@merykitty hotspot, shenandoah have been added to this pull request based on files touched in new commit(s).

rwestrel · 2025-12-16T16:51:27Z

src/hotspot/share/opto/graphKit.cpp

+    // dependency:
+    //  StoreC -> MemBar -> MergeMem -> compress_string -> MergeMem -> CharMem
+    //                               -------------------------------->
+    Node* all_mem = reset_memory();


This code sequence is used several times. Would it make sense to factor it out in its own method?

merykitty · 2025-12-16T17:30:12Z

/label remove shenandoah hotspot

openjdk · 2025-12-16T17:32:35Z

@merykitty
The shenandoah label was successfully removed.

The hotspot label was successfully removed.

rwestrel

Looks good to me.

openjdk · 2026-01-07T09:34:23Z

@merykitty 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 intrinsicsadrtype
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

dean-long · 2026-01-30T01:53:23Z

So after looking at this PR I have learned that C2 can control reordering of memory operations in at least 3 ways: anti-dependencies, memory slices, or membars. Are there are rules-of-thumb on which is best to use? Using a membar seems the most conservative but probably allows fewer optimizations.

By the way, I see that LibraryCallKit::inline_encodeISOArray and corresponding Java method do pretty much the same things a compress. So I tried adding a test for it in TestAntiDependency.java. But to my surprise, it passes, even without the fixes in this PR. I would expect it to fail, because the existing code uses TypeAryPtr::BYTES, so how does it prevent the movement of a char[] store in the test?

dean-long · 2026-01-30T02:12:15Z

Dumb question: why are these intrinsic nodes not implemented as MemNodes?

For nodes such as StrInflatedCopyNode, as they consume more than they produce, during scheduling, we need to compute anti-dependencies. This is not the case

Why is that?

so we should fix it by making the nodes kill all the memory they consume.

Why can't we use MergeMem and memory slices/aliases like regular load and store?

dean-long · 2026-01-30T02:38:57Z

This may be unrelated, but I checked to see if we treat Op_EncodeISOArray the same as Op_StrCompressedCopy everywhere. In two places in ConnectionGraph::split_unique_types, we treat them differently. For both we look at in(MemNode::Memory), but for Op_EncodeISOArray we also look at use->in(3). I don't understand this code well enough to decide if this a missing optimization or a correctness issue.

merykitty · 2026-01-30T02:53:07Z

@dean-long Thanks for taking a look.

So I tried adding a test for it in TestAntiDependency.java. But to my surprise, it passes, even without the fixes in this PR

I have added a test for this method. If it does not fail then adding -XX:+StressGCM -XX:+StressLCM may help.

Dumb question: why are these intrinsic nodes not implemented as MemNodes?

I think it is because only LoadNode and StoreNode are MemNode, even LoadStoreNode does not extend MemNode.

For nodes such as StrInflatedCopyNode, as they consume more than they produce, during scheduling, we need to compute anti-dependencies. This is not the case

Why is that?

During PhaseIdealLoop::get_late_ctrl, we only check the anti-dependency when a node returns true for is_Load():

  if (n->is_Load() && LCA != early) {
    LCA = get_late_ctrl_with_anti_dep(n->as_Load(), early, LCA);
  }

During PhaseCFG::schedule_late, we only check the anti-dependency when a node has the flag Flag_needs_anti_dependence_check set.

bool Node::needs_anti_dependence_check() const {
  if (req() < 2 || (_flags & Flag_needs_anti_dependence_check) == 0) {
    return false;
  }
  return in(1)->bottom_type()->has_memory();
}

We may fix these places, but since it is a really rare occurrence that a node consumes some memory and produces some but the latter is different from the former, so it is more reasonable to fix the graph at these nodes.

so we should fix it by making the nodes kill all the memory they consume.

Why can't we use MergeMem and memory slices/aliases like regular load and store?

Thanks to Roland's suggestion, now it only kills the 2 slices it concerns with and not the whole memory state.

merykitty · 2026-01-30T02:57:54Z

This may be unrelated, but I checked to see if we treat Op_EncodeISOArray the same as Op_StrCompressedCopy everywhere. In two places in ConnectionGraph::split_unique_types, we treat them differently. For both we look at in(MemNode::Memory), but for Op_EncodeISOArray we also look at use->in(3). I don't understand this code well enough to decide if this a missing optimization or a correctness issue.

I believe it is because before this change, EncodeISOArray does not consume the memory of the destination like StrCompressed, so it may miss being pushed on the worklist. As a result, checking for in(3) ensures the node is visited. After this change, EncodeISOArray correctly consumes the memory of its destination, so that become unnecessary.

dean-long · 2026-02-06T22:32:12Z

I'm still looking at this, but I'm getting confused by all the special cases. I wish C2 handling of memory was more uniform.

dean-long · 2026-02-06T22:33:35Z

It looks like we use SCMemProjNode for nodes that both read and write memory, so why is it not used for StrInflatedCopyNode in inflate_string?

merykitty · 2026-02-07T02:03:47Z

@dean-long SCMemProj is used for nodes that modify memory but still want to return a value. StrInflatedNode does not return a value so it does not need to use an SCMemProj, it can be the memory node itself.

Fix memory around intrinsics nodes

e978917

merykitty changed the title ~~8373591: C2: FIx the memory around some intrinsics nodes~~ 8373591: C2: Fix the memory around some intrinsics nodes Dec 12, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 12, 2025

openjdk bot added the rfr Pull request is ready for review label Dec 12, 2025

rwestrel reviewed Dec 12, 2025

View reviewed changes

Fix Shenandoah

1e02635

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Dec 12, 2025

Use MemBar instead of widening the intrinsic memory

9649a2f

rwestrel reviewed Dec 16, 2025

View reviewed changes

consolidate the memory effect into a function

c3503ed

openjdk bot removed shenandoah shenandoah-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Dec 16, 2025

rwestrel approved these changes Dec 17, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 17, 2025

merykitty added 2 commits January 7, 2026 16:27

Merge branch 'master' into intrinsicsadrtype

ad4f5cc

copyright year

45cc88c

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jan 7, 2026

Merge branch 'master' into intrinsicsadrtype

b871ba8

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jan 7, 2026

merykitty added 2 commits January 30, 2026 09:20

Merge branch 'master' into intrinsicsadrtype

eaaf479

Add test case for StringCoding::implEncodeAsciiArray

47f4bf3

Conversation

merykitty commented Dec 12, 2025 • 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 Dec 12, 2025

Uh oh!

merykitty commented Dec 12, 2025

Uh oh!

openjdk bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

rwestrel Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty Dec 12, 2025 • 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!

rwestrel Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 12, 2025

Uh oh!

rwestrel Dec 16, 2025 • 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!

merykitty Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty commented Dec 16, 2025

Uh oh!

openjdk bot commented Dec 16, 2025

Uh oh!

rwestrel left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 7, 2026

Uh oh!

dean-long commented Jan 30, 2026

Uh oh!

dean-long commented Jan 30, 2026

Uh oh!

dean-long commented Jan 30, 2026

Uh oh!

merykitty commented Jan 30, 2026

Uh oh!

merykitty commented Jan 30, 2026

Uh oh!

dean-long commented Feb 6, 2026

Uh oh!

dean-long commented Feb 6, 2026

Uh oh!

merykitty commented Feb 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

merykitty commented Dec 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 12, 2025 •

edited

Loading

openjdk bot commented Dec 12, 2025 •

edited

Loading

mlbridge bot commented Dec 12, 2025 •

edited

Loading

merykitty Dec 12, 2025 •

edited

Loading

rwestrel Dec 16, 2025 •

edited

Loading