8347481: C2: Remove the control input of some nodes #23055

merykitty · 2025-01-12T13:43:44Z

Hi,

While working on JDK-8347365, I noticed that there are some nodes that have their control inputs being set in a seemingly erroneous manner. This patch removes the control inputs for those nodes.

Please review this PR, 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-8347481: C2: Remove the control input of some nodes (Enhancement - P4)

Reviewers

Damon Fenacci (@dafedafe - Committer)
Vladimir Ivanov (@iwanowww - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23055

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-12T13:44:13Z

👋 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.

openjdk · 2025-01-12T13:44:15Z

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

8347481: C2: Remove the control input of some nodes

Reviewed-by: dfenacci, vlivanov, kvn

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

e7795cc: 8347347: Build fails undefined symbol: __asan_init by clang17
788f049: 8346607: IGV: Support drag-and-drop for opening graph files
bfa0cb7: 8347730: Replace SIZE_FORMAT in g1
3b6e576: 8347825: Make IDEA ide support use proper build system mechanisms
8bfa540: 8347731: Replace SIZE_FORMAT in zgc
cbe2f96: 8347732: Replace SIZE_FORMAT in shenandoah
f64f22b: 8346107: Generators: testing utility for random value generation
4ad5d55: 8325030: PhaseMacroExpand::value_from_mem_phi assert with "unknown node on this path"
24de9de: 8347370: Unnecessary Hashtable usage in javax.swing.text.html.HTML
d23ad01: 8337978: Verify OopHandles oops on access
... and 141 more: https://git.openjdk.org/jdk/compare/a641932427cbe8453130593355372837d70a098f...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-01-12T13:44:47Z

@merykitty The following labels will be automatically applied to this pull request:

graal
hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-01-12T13:48:02Z

Webrevs

00: Full (050f1a6c)

merykitty · 2025-01-12T13:48:12Z

/label remove graal

openjdk · 2025-01-12T13:49:18Z

@merykitty
The graal label was successfully removed.

dafedafe · 2025-01-13T14:26:15Z

Nice cleanup @merykitty. Thanks!
I was just wondering if there could be more nodes where the control input is wrongly set (possibly enough for a followup issue?)

merykitty · 2025-01-14T03:37:30Z

@dafedafe Thanks, there's LoadKlassNode which very rarely takes a control input but the reason given does not convince me. However, it seems to be a less clear cut compared to these nodes so I think a separate RFE is more suitable.

dafedafe

Fair enough. I happened to notice that some other nodes seemed to give the possibility of setting the control input although it might not make sense (e.g. some Div*Node and Mod*Node) and I thought it might be worth having a global look at this at some point.

This fix seems totally OK for me as it is now. Thanks again @merykitty.

vnkozlov · 2025-01-15T18:24:47Z

src/hotspot/share/opto/movenode.hpp

-  CMovePNode( Node *c, Node *bol, Node *left, Node *right, const TypePtr* t ) : CMoveNode(bol,left,right,t) { init_req(Control,c); }
+  CMovePNode(Node* bol, Node* left, Node* right, const TypePtr* t) : CMoveNode(bol, left, right, t) {}


I looked on history and this control setting was added at the very beginning.
May be it was done to avoid bypassing safepoints where oops could be modified (objects moved) and result of CMoveP before and after safepoint will be different.
I would be careful about changing it and CMoveN.

I have thought about it and can't think of a reason for the control input.
Firstly, the oop map is created after scheduling and should take care of this seemlessly.
Secondly, the control input does not prevent a CMoveP from moving down past a safepoint. And since these nodes have depends_only_on_test == true, they can be moved from a test to an equivalent dominating test, skipping any safepoint in between, which means these nodes can be moved up past a safepoint, too.
As a result, I don't think the control input of CMoveP and CMoveN is necessary and can be safely removed.

Any undesireable performance effects if CMove node floats up beyond its (original) control? It may have changed on modern hardware, but my recollection is cmov instructions were quite expensive to execute.

@iwanowww A cmov is as cheap as an add so I don't think there will be any performance effect. I believe what you mean by cmov being expensive is that it has 1 cycle of latency on both of its inputs, while a well-predicted if diamond has 0 cycle of latency on the chosen input only.

Right, my memory failed me. Indeed I was referring to relative slowness of cmov instructions compared to well-predicted conditional jumps which is not relevant here.

As a result, I don't think the control input of CMoveP and CMoveN is necessary and can be safely removed.

Okay. Let test it in JDK 25 and see how it goes. We have time to fix it if we find something wrong.
@dafedafe ran tier1-5 our testing and I don't see any new failures.

iwanowww

Looks good.

merykitty · 2025-01-17T02:04:09Z

Thanks a lot for your reviews and testing, let me integrate the patch.
/integrate

openjdk · 2025-01-17T02:05:17Z

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

1f365cc: 8346433: Cannot use DllMain in hotspot for static builds
0330ca4: 8347617: Shenandoah: Use consistent name for update references phase
07a0dcb: 8347576: Error output in libjsound has non matching format strings
d218b54: 8347609: Replace SIZE_FORMAT in os/os_cpu/cpu directories
e7795cc: 8347347: Build fails undefined symbol: __asan_init by clang17
788f049: 8346607: IGV: Support drag-and-drop for opening graph files
bfa0cb7: 8347730: Replace SIZE_FORMAT in g1
3b6e576: 8347825: Make IDEA ide support use proper build system mechanisms
8bfa540: 8347731: Replace SIZE_FORMAT in zgc
cbe2f96: 8347732: Replace SIZE_FORMAT in shenandoah
... and 145 more: https://git.openjdk.org/jdk/compare/a641932427cbe8453130593355372837d70a098f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-17T02:05:25Z

@merykitty Pushed as commit aa21de5.

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

remove control inputs from several nodes

050f1a6

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

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jan 12, 2025

openjdk bot removed the graal graal-dev@openjdk.org label Jan 12, 2025

dafedafe approved these changes Jan 15, 2025

View reviewed changes

vnkozlov reviewed Jan 15, 2025

View reviewed changes

iwanowww approved these changes Jan 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2025

vnkozlov approved these changes Jan 16, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 17, 2025

openjdk bot closed this Jan 17, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8347481: C2: Remove the control input of some nodes #23055

8347481: C2: Remove the control input of some nodes #23055

merykitty commented Jan 12, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 12, 2025

openjdk bot commented Jan 12, 2025 •

edited

Loading

openjdk bot commented Jan 12, 2025

mlbridge bot commented Jan 12, 2025

merykitty commented Jan 12, 2025

openjdk bot commented Jan 12, 2025

dafedafe commented Jan 13, 2025

merykitty commented Jan 14, 2025

dafedafe left a comment

vnkozlov Jan 15, 2025

merykitty Jan 15, 2025

iwanowww Jan 15, 2025

merykitty Jan 16, 2025

iwanowww Jan 16, 2025

vnkozlov Jan 16, 2025

iwanowww left a comment

merykitty commented Jan 17, 2025

openjdk bot commented Jan 17, 2025

openjdk bot commented Jan 17, 2025

		CMovePNode( Node c, Node bol, Node left, Node right, const TypePtr* t ) : CMoveNode(bol,left,right,t) { init_req(Control,c); }
		CMovePNode(Node* bol, Node* left, Node* right, const TypePtr* t) : CMoveNode(bol, left, right, t) {}

8347481: C2: Remove the control input of some nodes #23055

8347481: C2: Remove the control input of some nodes #23055

Conversation

merykitty commented Jan 12, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 12, 2025

openjdk bot commented Jan 12, 2025 • edited Loading

openjdk bot commented Jan 12, 2025

mlbridge bot commented Jan 12, 2025

Webrevs

merykitty commented Jan 12, 2025

openjdk bot commented Jan 12, 2025

dafedafe commented Jan 13, 2025

merykitty commented Jan 14, 2025

dafedafe left a comment

Choose a reason for hiding this comment

vnkozlov Jan 15, 2025

Choose a reason for hiding this comment

merykitty Jan 15, 2025

Choose a reason for hiding this comment

iwanowww Jan 15, 2025

Choose a reason for hiding this comment

merykitty Jan 16, 2025

Choose a reason for hiding this comment

iwanowww Jan 16, 2025

Choose a reason for hiding this comment

vnkozlov Jan 16, 2025

Choose a reason for hiding this comment

iwanowww left a comment

Choose a reason for hiding this comment

merykitty commented Jan 17, 2025

openjdk bot commented Jan 17, 2025

openjdk bot commented Jan 17, 2025

merykitty commented Jan 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 12, 2025 •

edited

Loading