8347901: C2 should remove unused leaf / pure runtime calls #25760

marc-chevalier · 2025-06-11T16:18:41Z

A first part toward a better support of pure functions, but this time, with guidance from @iwanowww.

Pure Functions

Pure functions (considered here) are functions that have no side effects, no effect on the control flow (no exception or such), cannot deopt etc.. It's really a function that you can execute anywhere, with whichever arguments without effect other than wasting time. Integer division is not pure as dividing by zero is throwing. But many floating point functions will just return NaN or +/-infinity in problematic cases.

Scope

We are not going all powerful for now! It's mostly about identifying some pure functions and being able to remove them if the result is unused. Some other things are not part of this PR, on purpose. Especially, this PR doesn't propose a way to move pure calls around. The reason is that pure calls are later expanded into regular calls, which require a control input. To be able to do the expansion, we just keep the control in the pure call as well.

Implementation Overview

We created here some new node kind for pure calls, inheriting leaf calls, that are expanded into regular leaf calls during final graph reshaping. The possibility to support pure call directly in AD file is left open.

This PR also introduces TupleNode (largely based on an original idea/implem of @iwanowww), that just tie multiple input together and play well with ProjNode: the n-th projection of a TupleNode is the n-th input of the tuple. This is a convenient way to skip and remove nodes from the graph while delegating the difficulty of the surgery to the trusted IGVN's implementation.

Thanks,
Marc

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-8347901: C2 should remove unused leaf / pure runtime calls (Enhancement - P3)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25760

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-11T16:19:44Z

👋 Welcome back mchevalier! 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-06-11T16:20:01Z

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

8347901: C2 should remove unused leaf / pure runtime calls

Reviewed-by: thartmann, vlivanov

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

eceb3bb: 8362452: [macOS] Remove CPrinterJob.finalize()
3acdba3: 8362557: [macOS] Remove CFont.finalize()
523993e: 8362291: [macOS] Remove finalize method in CGraphicsEnvironment.java
... and 487 more: https://git.openjdk.org/jdk/compare/bf7d40d0486b7b4e4820bb5d08a63c446ea3291d...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-06-11T16:20:42Z

@marc-chevalier 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-06-11T16:26:58Z

Webrevs

TheRealMDoerr · 2025-06-11T16:49:40Z

Just a side comment: "Integer division is not pure as dividing by zero is throwing." is only true for some platforms. See JDK-8299857.

marc-chevalier · 2025-06-11T16:54:23Z

Right. Yet, it's safe to consider it as non-pure, as an over-approximation, but it could be refined.

eme64

@marc-chevalier Nice work!

I have a first set of qestions / suggestions :)

src/hotspot/share/opto/callnode.cpp

src/hotspot/share/opto/compile.cpp

src/hotspot/share/opto/divnode.cpp

src/hotspot/share/opto/graphKit.cpp

eme64 · 2025-06-18T14:44:26Z

src/hotspot/share/opto/multnode.cpp

+    if (ctrl->Opcode() == Op_Tuple) {
+      // Jumping over Tuples: the i-th projection of a Tuple is the i-th input of the Tuple.
+      ctrl = ctrl->in(_con);
+    }


Do you need to special-case this here? Why does the ProjNode::Identity not suffice? Are there potentially other locations where we now would need this special logic?

That is a good question. That is something I picked from Vladimir's implementation and it seemed legitimate. But now you say it, is it needed? Not sure. I'm trying to find that out. Would ::Identity be enough? It's tempting to say so, right! I'd say it can be not enough if we need adr_type before idealizing the ProjNode (no idea if that happens). Is there any other places to adapt? One could think so, but actually, I can't find such an example. Other methods of ProjNode for instance rely on the type of the input (which is correctly handled in TupleNode), and so should already work fine.

I'm trying to understand what happens if we don't have that. But maybe @iwanowww would have some helpful insight?

I don't remember all the details now, but there were some problems when ProjNode::adr_type() encounters TupleNode.

I've run some test without this special handling and I couldn't see nothing more failing. Maybe it was more necessary in Vladimir's original usecase (reachability). In my case, maybe I don't need adr_type to handle nicely TupleNodes, but let's not set a trap for the future.

src/hotspot/share/opto/multnode.cpp

src/hotspot/share/opto/multnode.hpp

eme64

@marc-chevalier Thanks for addressing my comments! I now have a few more for you :)

src/hotspot/share/opto/callnode.cpp

eme64 · 2025-06-20T15:42:53Z

src/hotspot/share/opto/callnode.cpp

+}
+
+Node* CallLeafPureNode::Ideal(PhaseGVN* phase, bool can_reshape) {
+  if (is_dead()) { // dead node


Suggested change

if (is_dead()) { // dead node

if (is_dead()) {

The comment seemed redundant. You could say who else is responsible of cleaning up the dead node though.

What would happen if the CallLeafPureNode loses its control projection, but not the other uses? I don't even know if that is possible. What do you think?

IGVN takes care of removing a node without output.

This was motivated by https://bugs.openjdk.org/browse/JDK-8353341 I think @TobiHartmann told me it's common not to touch dead nodes during idealization.

I think it is possible to lose control projection but not data because data was already found dead, but data wasn't yet (but should happen shortly after). As soon as the data projection disappear, the node should be removed. remove_dead_node (used in IGVN) is aggressively removing a dead node and all the usages that become recursively dead. It's the classic constraint of having to find that both data and control are top when one is.

src/hotspot/share/opto/callnode.cpp

eme64 · 2025-06-20T15:49:25Z

src/hotspot/share/opto/callnode.hpp

 #endif
 };

+class CallLeafPureNode : public CallLeafNode {


You need a short description about what this node is for. What are the assumptions about it?

eme64 · 2025-06-20T15:50:46Z

src/hotspot/share/opto/divnode.cpp

  if (!can_reshape) {
    return nullptr;
  }


Would this prevent us from doing the make_tuple_of_input_state_and_top_return_values trick? Because it seems to me that we do not need to reshape the node for that, right? Maybe you should reorder things for that?

Also, you should probably call CallLeafPureNode::Ideal instead of duplicating its logic here and in other subclasses.

In an earlier issue (JDK-8349523), I tried to remove these nodes during parsing. It didn't work well. The problem is that it's transformed by GVN before setting any output projection, so of course, the node is removing itself before having the opportunity of being used. We wait until usages have been set before we try to remove the node (or replace it with a tuple).

That sounds like a bug at the use-site... don't you think?

As discussed offline, it's relatively normal. Adding a comment to explain.

src/hotspot/share/opto/callnode.cpp

benoitmaillard · 2025-07-02T14:35:26Z

src/hotspot/share/opto/parse2.cpp

 Node* Parse::floating_point_mod(Node* a, Node* b, BasicType type) {
  assert(type == BasicType::T_FLOAT || type == BasicType::T_DOUBLE, "only float and double are floating points");
-  CallNode* mod = type == BasicType::T_DOUBLE ? static_cast<CallNode*>(new ModDNode(C, a, b)) : new ModFNode(C, a, b);
+  CallLeafPureNode* mod = type == BasicType::T_DOUBLE ? static_cast<CallLeafPureNode*>(new ModDNode(C, a, b)) : new ModFNode(C, a, b);


May I ask why we only need the static_cast for the ModDNode here?

It's C/C++ being annoying here: both branches of the ternary must have the same type, or something compatible. If I remove the cast:

error: conditional expression between distinct pointer types 'ModDNode*' and 'ModFNode*' lacks a cast

With the case, C++ can converrt the ModFNode* into a CallLeafPureNode* just fine.

I didn't invent the cast, it was here before, but good to question it.

TobiHartmann

Thanks for digging into this Marc. The changes look good to me. I just added a few minor comments / questions.

this PR doesn't propose a way to move pure calls around

Should we have a separate RFE for that?

src/hotspot/share/opto/graphKit.cpp

TobiHartmann · 2025-07-07T07:53:24Z

src/hotspot/share/opto/divnode.cpp

+  Node* super = CallLeafPureNode::Ideal(phase, can_reshape);
+  if (super != nullptr) {
+    return super;


Can't we just do return CallLeafPureNode::Ideal(phase, can_reshape); at the end of ModFNode::Ideal instead of return nullptr? That's what we usually do in C2, for example in CallStaticJavaNode::Ideal -> CallNode::Ideal. Feels more natural to me and would avoid the super != nullptr check. Also for the other Ideal methods that you modified.

We could but it's not that direct. ModFNode::Ideal has 6 returns (without mine):

3 are return replace_with_con(...); which in their turn return nullptr but after making changes in the graph.

2 are return nullptr;

1 is actually returning a node.
And especially the final one is

return replace_with_con(igvn, TypeF::make(jfloat_cast(xr)));

If we change replace_with_con to actually return a TupleNode to do the job, we still have 2 places where to call the base class' Ideal. So I'm not sure how much better it would be to duplicate the call. It also adds a maintenance burden: if one adds another case where we don't want to make changes, one needs to add another call to CallLeafPureNode::Ideal. I think it's because of the structure of this function: rather than selecting cases where we want to do something and reaching the end with only the leftover cases, we select the cases we don't want to continue with, and we return early, making more cases where we should call the super method.

I'll try something.

Ah, makes sense. Feel free to leave as-is then.

There, you can see the thing I've tried. It changes a bit more code, but overall, I think it makes it clearer and address your comment.

TobiHartmann · 2025-07-07T08:30:00Z

src/hotspot/share/opto/divnode.cpp

-  bool result_is_unused = proj_out_or_null(TypeFunc::Parms) == nullptr;
-  bool not_dead = proj_out_or_null(TypeFunc::Control) != nullptr;
-  if (result_is_unused && not_dead) {
-    return replace_with_con(igvn, TypeF::make(0.));


Can we replace all the other usages of ModFloatingNode::replace_with_con by TupleNode and get rid of that method?

Yes, that sounds like a good idea. One still need to build the right TupleNode, which takes a bit of code. So in detail, I'd rather replace replace_with_con with a function returning the right TupleNode to be as concise on the call-site.

Yes, that sounds good.

TobiHartmann

Thanks for making these changes, I like that version more.

marc-chevalier · 2025-07-10T10:46:11Z

@iwanowww would you like to take a look at it, since you have quite some context already?

iwanowww

Looks good.

marc-chevalier · 2025-07-22T08:47:02Z

/integrate

Thanks all for your comments!

openjdk · 2025-07-22T08:48:16Z

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

f155661: 8342692: C2: long counted loop/long range checks: don't create loop-nest for short running loops
c68697e: 8362957: Fix jdk/javadoc/doccheck/checks/jdkCheckHtml.java (docs) failure
dccb178: 8361892: AArch64: Incorrect matching rule leading to improper oop instruction encoding
... and 493 more: https://git.openjdk.org/jdk/compare/bf7d40d0486b7b4e4820bb5d08a63c446ea3291d...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-22T08:48:27Z

@marc-chevalier Pushed as commit ed70910.

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

Eliminate pure function calls

ff0a6b6

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jun 11, 2025

marc-chevalier marked this pull request as ready for review June 11, 2025 16:23

openjdk bot added the rfr Pull request is ready for review label Jun 11, 2025

eme64 suggested changes Jun 18, 2025

View reviewed changes

Mostly comments

34fd5e9

eme64 suggested changes Jun 20, 2025

View reviewed changes

mostly comments

7028b56

benoitmaillard reviewed Jul 3, 2025

View reviewed changes

TobiHartmann approved these changes Jul 7, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 7, 2025

typo

7f18c9f

openjdk bot removed the ready Pull request is ready to be integrated label Jul 8, 2025

Tentative to address Tobias' comments

7553e30

TobiHartmann approved these changes Jul 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 9, 2025

iwanowww approved these changes Jul 21, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 22, 2025

openjdk bot closed this Jul 22, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Jul 22, 2025

openjdk bot removed the rfr Pull request is ready for review label Jul 22, 2025

8347901: C2 should remove unused leaf / pure runtime calls #25760

8347901: C2 should remove unused leaf / pure runtime calls #25760

Uh oh!

Conversation

marc-chevalier commented Jun 11, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pure Functions

Scope

Implementation Overview

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 11, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jun 11, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

TheRealMDoerr commented Jun 11, 2025

Uh oh!

marc-chevalier commented Jun 11, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

marc-chevalier commented Jun 11, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 11, 2025 •

edited

Loading

mlbridge bot commented Jun 11, 2025 •

edited

Loading