8351414: C2: MergeStores must happen after RangeCheck smearing #23944

eme64 · 2025-03-07T15:07:37Z

With JDK-8348959 we see that there can be some issues when RangeCheck smearing happens in the same IGVN phase as MergeStores. It means that some RangeChecks are still around as we do MergeStores, and then we cannot merge as many stores as we would like. We should ensure that RangeCheck smearing happens during post-loop-opts, and then MergeStores happens in a separate dedicated IGVN round afterwards.

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-8351414: C2: MergeStores must happen after RangeCheck smearing (Enhancement - P4)

Reviewers

Quan Anh Mai (@merykitty - Committer) Review applies to 4667e4bc
Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23944

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-07T15:08:21Z

👋 Welcome back epeter! 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 · 2025-03-07T15:08:29Z

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

8351414: C2: MergeStores must happen after RangeCheck smearing

Reviewed-by: chagedorn, qamai

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 26 new commits pushed to the master branch:

d90b79a: 8351046: Rename ObjectMonitor functions
e90b6bd: 8350638: Make keyboard navigation more usable in API docs
4867a4c: 8351280: Mark Assertion Predicates useless instead of replacing them by a constant directly
64caf08: 8350572: ZGC: Enhance z_verify_safepoints_are_blocked interactions with VMError
fb0efbe: 8333578: Fix uses of overaligned types induced by ZCACHE_ALIGNED
99547c5: 8346825: [JVMCI] Remove NativeImageReinitialize annotation
ec683a1: 8351419: java.net.http: Cleanup links in HttpResponse and module-info API doc comments
f61f520: 8350325: [PPC64] ConvF2HFIdealizationTests timeouts on Power8
783eda9: 8350266: [PPC64] Interpreter: intrinsify Thread.currentThread()
19b9f11: 8351392: C2 crash: failed: Expected Bool, but got OpaqueMultiversioning
... and 16 more: https://git.openjdk.org/jdk/compare/7314efc9483c5db6ecccd9215c04d78818e6a9a2...master

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 master branch, type /integrate in a new comment.

openjdk · 2025-03-07T15:09:11Z

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

merykitty · 2025-03-07T15:47:33Z

Do you think it would make sense to make a dedicated PhaseMergeStores instead?

eme64 · 2025-03-07T15:54:51Z

Do you think it would make sense to make a dedicated PhaseMergeStores instead?

Hmm, maybe? Can you say a little more how you imagine it?

eme64 · 2025-03-07T16:00:47Z

@merykitty I basically just copied the structure from post_loop_opts_phase...

merykitty · 2025-03-07T16:02:43Z

I would imagine PhaseMergeStores that looks at all stores in the graph, does the analysis and the transformation, then we can run another round of IGVN after that. I think a global view would be easier and more efficient than the local view from StoreNode::Ideal. Since StoreNode::Ideal needs to ensure that there is no store after it and just bail out otherwise.

eme64 · 2025-03-07T16:13:01Z

@merykitty I'm not sure if I understood you right. You seem to say that I should do this (please correct me):

Traverse the WHOLE graph, looking for stores (I would like to avoid that, that's why I carry around the list).
Rewrite the logic in MergeStores, and somehow have a global view rather than the local one. That's lots of work and I'm not sure I want to invest the time, unless it is somehow clearly better.

This is really a fix for Valhalla, see https://bugs.openjdk.org/browse/JDK-8348959, so it would be nice to fix this rather sooner. If someone wants to then refactor the code later, that's fine with me ;)

merykitty · 2025-03-07T16:28:56Z

@eme64

Traverse the WHOLE graph, looking for stores (I would like to avoid that, that's why I carry around the list).

I think keeping a list is fine.

Rewrite the logic in MergeStores, and somehow have a global view rather than the local one. That's lots of work and I'm not sure I want to invest the time, unless it is somehow clearly better.

I think the logic would still be the same. We start at a store then try to find the last store in the chain, then group the stores and do the merge. After that, we can remove the replaced stores from the work list. It is global in the sense that we can freely walk the graph instead of being restricted to the current node that is invoking Ideal. This leads to a series of:

Status status_use = find_adjacent_use_store(_store);
if (status_use.found_store() != nullptr) {
  return nullptr;
}

while we can do

while (next != nullptr) {
  StoreNode* last = next;
  next = find_adjacent_use_store(last);
}

It is also better because idealisation of a store node may be invoked several times, leading to useless find_adjacent_use_store invocations.

kuaiwei · 2025-03-10T01:58:23Z

@eme64 I'm working on merging loads and I meet the same problem. I work around it by delay transform LoadNode after all range check smearing. But I think your solution is better. Could you change the name "_for_merge_stores_igvn" as "_for_merge_mem_igvn" and it can be used both for store and loads?

eme64 · 2025-03-10T10:50:11Z

@merykitty Thanks for giving more details. I agree that your idea would lead to some fewer adjacency checks, and so it would be somewhat desirable to do that. However, splitting out the merge_stores list would have to be done anyway, and that is almost all of the code I have here, so this here is a step in the right direction. Using IGVN is fine in my view, especially because I'm not newly introducing it here, but just moving it.

I personally have decided to only put minimal effort into MergeStores, my priorities are elsewhere. This issue here is primarily addressing the problems in the Valhalla repo, where the issues with the order of RC smearing and MergeStores is somehow more apparent than on mainline. So if you feel that this idea is important to you, feel free to file an RFE, maybe someone else wants to take this effort on.

@kuaiwei I think for now I'll leave it with merge_stores, but once you implement the merge_loads, you can just rename it. I'm not sure yet what would be the best name. merge_mem reminds me too much of the MergeMemNode. Maybe merge_loads_and_stores or merge_memops could be a better alternative.

mlbridge · 2025-03-10T12:04:28Z

Webrevs

01: Full - Incremental (41292a65)
00: Full (4667e4bc)

merykitty

Thanks for the elaboration. LGTM.

chhagedorn

Looks good!

chhagedorn · 2025-03-10T13:44:14Z

src/hotspot/share/opto/compile.cpp

+// We need to wait with merging stores until RangeCheck smearing has removed the RangeChecks during
+// the post loops IGVN phase. If we do it earlier, then there may still be some RangeChecks between
+// the stores, and we merge the wrong sequence of stores.
+// Example:
+//   StoreI RangeCheck StoreI StoreI RangeCheck StoreI
+// Apply MergeStores:
+//   StoreI RangeCheck [   StoreL  ] RangeCheck StoreI
+// Remove more RangeChecks:
+//   StoreI            [   StoreL  ]            StoreI
+// But now it would have been better to do this instead:
+//   [         StoreL       ] [       StoreL         ]


Maybe you also can add a note here that RC smearing is not limited to just this one IGVN phase but that it's done in all subsequent IGVN phases (since we don't unset _merge_stores_phase).

I added a comment below :)

src/hotspot/share/opto/compile.cpp

eme64 · 2025-03-11T07:09:40Z

Thanks @chhagedorn @merykitty for the reviews!

/integrate

openjdk · 2025-03-11T07:10:32Z

Going to push as commit 4cf6316.
Since your change was applied there have been 38 commits pushed to the master branch:

8a5ed47: 8350148: Native stack overflow when writing Java heap objects into AOT cache
5928209: 8347405: MergeStores with reverse bytes order value
f984c2b: 8351505: (fs) Typo in the documentation of java.nio.file.spi.FileSystemProvider.getFileSystem()
ffa6340: 8351567: Jar Manifest test ValueUtf8Coding produces misleading diagnostic output
8d8bd0c: 8349492: Update sun/security/pkcs12/KeytoolOpensslInteropTest.java to use a recent Openssl version
73465b9: 8160327: Support for thumbnails present in APP1 marker for JPEG
dbdbbd4: 8348597: Update HarfBuzz to 10.4.0
7999091: 8351555: Help section added in JDK-8350638 uses invalid HTML
8450ae9: 8351440: Link with -reproducible on macOS
b40be22: 8333393: PhaseCFG::insert_anti_dependences can fail to raise LCAs and to add necessary anti-dependence edges
... and 28 more: https://git.openjdk.org/jdk/compare/7314efc9483c5db6ecccd9215c04d78818e6a9a2...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-11T07:10:40Z

@eme64 Pushed as commit 4cf6316.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot changed the title ~~JDK-8351414~~ 8351414: C2: MergeStores must happen after RangeCheck smearing Mar 7, 2025

openjdk bot added the hotspot-compiler label Mar 7, 2025

more fix, and more comments

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

Loading
Loading status checks…

e6612fb

clear flag after clone

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

Loading
Loading status checks…

22dce9f

eme64 marked this pull request as ready for review March 10, 2025 11:59

openjdk bot added the rfr label Mar 10, 2025

merykitty approved these changes Mar 10, 2025

View reviewed changes

chhagedorn approved these changes Mar 10, 2025

View reviewed changes

openjdk bot added the ready label Mar 10, 2025

eme64 commented Mar 10, 2025

View reviewed changes

src/hotspot/share/opto/compile.cpp Show resolved Hide resolved

openjdk bot removed the ready label Mar 10, 2025

chhagedorn approved these changes Mar 10, 2025

View reviewed changes

openjdk bot added the ready label Mar 10, 2025

openjdk bot added the integrated label Mar 11, 2025

openjdk bot closed this Mar 11, 2025

openjdk bot removed ready rfr labels Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8351414: C2: MergeStores must happen after RangeCheck smearing #23944

8351414: C2: MergeStores must happen after RangeCheck smearing #23944

eme64 commented Mar 7, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 7, 2025

openjdk bot commented Mar 7, 2025 •

edited

Loading

openjdk bot commented Mar 7, 2025

merykitty commented Mar 7, 2025

eme64 commented Mar 7, 2025

eme64 commented Mar 7, 2025

merykitty commented Mar 7, 2025

eme64 commented Mar 7, 2025 •

edited

Loading

merykitty commented Mar 7, 2025

kuaiwei commented Mar 10, 2025

eme64 commented Mar 10, 2025

mlbridge bot commented Mar 10, 2025 •

edited

Loading

merykitty left a comment

chhagedorn left a comment

chhagedorn Mar 10, 2025

eme64 Mar 10, 2025

eme64 commented Mar 11, 2025

openjdk bot commented Mar 11, 2025

openjdk bot commented Mar 11, 2025

8351414: C2: MergeStores must happen after RangeCheck smearing #23944

8351414: C2: MergeStores must happen after RangeCheck smearing #23944

Conversation

eme64 commented Mar 7, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 7, 2025

openjdk bot commented Mar 7, 2025 • edited Loading

openjdk bot commented Mar 7, 2025

merykitty commented Mar 7, 2025

eme64 commented Mar 7, 2025

eme64 commented Mar 7, 2025

merykitty commented Mar 7, 2025

eme64 commented Mar 7, 2025 • edited Loading

merykitty commented Mar 7, 2025

kuaiwei commented Mar 10, 2025

eme64 commented Mar 10, 2025

mlbridge bot commented Mar 10, 2025 • edited Loading

Webrevs

merykitty left a comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Mar 10, 2025

Choose a reason for hiding this comment

eme64 Mar 10, 2025

Choose a reason for hiding this comment

eme64 commented Mar 11, 2025

openjdk bot commented Mar 11, 2025

openjdk bot commented Mar 11, 2025

eme64 commented Mar 7, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 7, 2025 •

edited

Loading

eme64 commented Mar 7, 2025 •

edited

Loading

mlbridge bot commented Mar 10, 2025 •

edited

Loading