8353551: C2: Constant folding for ReverseBytes nodes #24382

SirYwell · 2025-04-02T16:17:57Z

This change implements constant folding for ReverseBytes nodes.

Currently, byteswap is included transitively by reverse_bits.hpp. I'm not sure if this is fine or if I need to add an explicit include there.

I appreciate any reviews and comments.

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-8353551: C2: Constant folding for ReverseBytes nodes (Enhancement - P4)

Reviewers

Vladimir Ivanov (@iwanowww - Reviewer) Review applies to 278a2a7c
Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24382

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-02T16:19:06Z

👋 Welcome back hgreule! 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-04-02T16:20:30Z

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

8353551: C2: Constant folding for ReverseBytes nodes

Reviewed-by: epeter, 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 682 new commits pushed to the master branch:

d7cb933: 8356593: RISC-V: Small improvement to array fill stub
74f047b: 8356224: JFR: Default value of @registered is ignored
97d2a37: 8355790: Enhance code consistency: java.desktop/unix:sun.awt
... and 679 more: https://git.openjdk.org/jdk/compare/d32ff1392205ea0fd179478a7ddb3d5f63923461...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@iwanowww, @eme64) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-04-02T16:21:32Z

@SirYwell 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-04-02T16:24:22Z

Webrevs

j3graham · 2025-04-04T17:22:34Z

src/hotspot/share/opto/subnode.cpp

 }

+template<typename T, BasicType B>
+const Type* reverse_bytes(const Node* node, PhaseGVN* phase) {


Could this be templated with TypeLong/TypeInt instead of BasicType? There is a try_cast that @merykitty added in #17508 that might help.

It wouldn't change much, but yes. Generally this is an example where #17508 shines, as the code could be generalized to just reverse the bytes of the KnownBits structure. Whether this PR waits until then or we refactor the code afterwards isn't that important to me.

I didn’t intend to advocate for waiting for that PR. try_cast is tiny and could be added independently. I have just been looking for things that could generally help with unifying TypeInt/TypeLong implementations.

Personally, I'd prefer to see a unified generic version of ReverseBytesNode::Value() rather than multiple specializations (even templated ones).

@j3graham I mainly want to avoid conflicts or duplicated solutions. I meant that after #17508 this code can be further generalized anyway, allowing to use the try_cast function then. This can obviously happen in a separate PR, if this one is integrated before.

@iwanowww I'm not sure if that's possible without more duplication. We need to choose the correct byteswap implementation depending on the node's type. Please let me know if I'm missing something.

Okay, I see. That works for me. One problem though, there currently isn't a common ReverseBytesNode type, so I'd need to add that. And I assume that should be a TypeNode then? In that case, as ReverseBytes*Nodes are InvolutionNodes, would InvolutionNode need to be a TypeNode? Or can I use multiple inheritance? (I didn't see any example of that in current Node types). Both are doable, and the first might even make sense, but I'm not sure if it's a bit much for this PR.

Or do you want me to (temporarily) duplicate the Value code (i.e. move more code from the templated function to the Value functions, and only keep the simple reverse_bytes from your snippet)?

Alternatively, I'd also be fine to put this PR on hold and make InvolutionNode a TypeNode first, or whatever you think is best. Please let me know what you think.

And I assume that should be a TypeNode then?

Why do you think it benefits from becoming a TypeNode?

In that case, as ReverseBytes*Nodes are InvolutionNodes, would InvolutionNode need to be a TypeNode? Or can I use multiple inheritance?

We try to avoid multiple inheritance in JVM and C2 doesn't use any AFAIK.

Actually, I had an afterthought about InvolutionNode after approving it. It looks a bit weird to model "involution" property through inheritance. (Primarily, because it's hard to mix multiple properties.) Node flags would be a better fit IMO.

For now, I suggest to just add a superclassReverseBytesNode which extends InvolutionNode and place Value there.

Why do you think it benefits from becoming a TypeNode?

Oh yeah I somehow confused myself there successfully.

Actually, I had an afterthought about InvolutionNode after approving it. It looks a bit weird to model "involution" property through inheritance. (Primarily, because it's hard to mix multiple properties.) Node flags would be a better fit IMO.

That would work, although it would make the common implementation more difficult I think. Also AddNode similarly models the "addition in a (semi)-ring" property, but there's clearly more shared code there (and the property can't be modeled as a flag there because the respective multiplicative operation is defined there too).

@iwanowww I applied your suggestion now. Please let me know if this is good now.

iwanowww · 2025-04-08T01:21:57Z

src/hotspot/share/opto/subnode.cpp

 }

+template<typename T, BasicType B>
+const Type* reverse_bytes(const Node* node, PhaseGVN* phase) {


Personally, I'd prefer to see a unified generic version of ReverseBytesNode::Value() rather than multiple specializations (even templated ones).

iwanowww · 2025-04-08T01:26:26Z

test/hotspot/jtreg/compiler/c2/irTests/ReverseBytesConstantsTests.java

+
+    @DontCompile
+    public void assertResultI() {
+        Asserts.assertEQ(Integer.reverseBytes(0x04030201), testI1());


Please, add more test cases (specifically, with negative constants).

I added cases with a leading 0x80 byte now

eme64 · 2025-04-08T16:59:26Z

@SirYwell This looks generally good, but I'll let you have the conversation with @iwanowww .
I launched some internal testing.

iwanowww

Overall, looks good. Submitted it for testing.

iwanowww · 2025-04-10T19:36:34Z

src/hotspot/share/opto/subnode.cpp

  return TypeH::make((float)sqrt((double)f));
 }

+const Type* reverse_bytes(int opcode, const Type* con) {


Please, declare it as static.

iwanowww

Testing results are good.

SirYwell · 2025-04-14T06:43:01Z

@iwanowww thanks, I addressed your last comment.

iwanowww

Looks good.

SirYwell · 2025-04-15T06:36:33Z

Thank you @iwanowww. @eme64 could you have another look too?

eme64

Nice, the code looks even better now :)

eme64 · 2025-04-17T09:11:55Z

test/hotspot/jtreg/compiler/c2/irTests/ReverseBytesConstantsTests.java

+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package compiler.c2.irTests;


Can you please move the test to a more specific directory?
The irTests directory was a bit of a mistake. I think this would make more sense in test/hotspot/jtreg/compiler/c2/gvn/.

eme64 · 2025-04-17T09:22:42Z

@iwanowww I see you did some internal testing, but not for what version. Should we re-run testing?

SirYwell · 2025-04-25T08:45:43Z

Sorry for the delay, I moved the test now @eme64. I think @iwanowww ran the tests after I changed the implementation to his suggestion. Please let me know when you think we can integrate.

eme64

It looks good to me.

But before you integrate:
I also launched testing again, just in case. Please ping me again in 24h for the results :)

SirYwell · 2025-05-09T10:47:02Z

Thanks for testing again @eme64. Are the results in?

eme64 · 2025-05-09T12:24:34Z

@SirYwell They are almost completed, but so far no failure :)

SirYwell · 2025-05-12T06:13:15Z

@eme64 just to make sure, I assume we can integrate this now?

eme64 · 2025-05-12T06:24:14Z

@SirYwell Yes, it is all green 🟢 Ship it :) 🚢

SirYwell · 2025-05-12T06:37:06Z

Thanks for your reviews!

/integrate

openjdk · 2025-05-12T06:37:37Z

@SirYwell
Your change (at version 3a94bbe) is now ready to be sponsored by a Committer.

eme64 · 2025-05-12T06:43:23Z

@SirYwell Thanks for the work :)
/sponsor

openjdk · 2025-05-12T06:43:53Z

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

d7cb933: 8356593: RISC-V: Small improvement to array fill stub
74f047b: 8356224: JFR: Default value of @registered is ignored
97d2a37: 8355790: Enhance code consistency: java.desktop/unix:sun.awt
... and 679 more: https://git.openjdk.org/jdk/compare/d32ff1392205ea0fd179478a7ddb3d5f63923461...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-12T06:44:02Z

@eme64 @SirYwell Pushed as commit de801fe.

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

Implement constant folding for ReverseBytes*Nodes

51d0a5d

openjdk bot changed the title ~~JDK-8353551~~ 8353551: C2: Constant folding for ReverseBytes nodes Apr 2, 2025

openjdk bot added the rfr Pull request is ready for review label Apr 2, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 2, 2025

j3graham reviewed Apr 4, 2025

View reviewed changes

iwanowww reviewed Apr 8, 2025

View reviewed changes

add test cases with negative -> non-negative

dc04676

introduce common ReverseBytesNode

90f2e9d

iwanowww reviewed Apr 10, 2025

View reviewed changes

iwanowww reviewed Apr 11, 2025

View reviewed changes

make function static

278a2a7

iwanowww approved these changes Apr 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 14, 2025

eme64 reviewed Apr 17, 2025

View reviewed changes

move test

f0ec0e4

openjdk bot removed the ready Pull request is ready to be integrated label Apr 25, 2025

correct driver path

3a94bbe

eme64 approved these changes May 8, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 8, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label May 12, 2025

openjdk bot added the integrated Pull request has been integrated label May 12, 2025

openjdk bot closed this May 12, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 12, 2025

SirYwell deleted the cf-ReverseBytes branch May 12, 2025 17:51

mhaessig mentioned this pull request Jul 31, 2025

8364409: [BACKOUT] Consolidate Identity of self-inverse operations #26570

Closed

5 tasks

Uh oh!

8353551: C2: Constant folding for ReverseBytes nodes #24382

8353551: C2: Constant folding for ReverseBytes nodes #24382

Uh oh!

Conversation

SirYwell commented Apr 2, 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 Apr 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Apr 2, 2025

Uh oh!

mlbridge bot commented Apr 2, 2025 • 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!

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!

SirYwell Apr 10, 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!

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!

eme64 commented Apr 8, 2025

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

SirYwell commented Apr 14, 2025

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

SirYwell commented Apr 15, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 commented Apr 17, 2025

Uh oh!

SirYwell commented Apr 25, 2025

Uh oh!

eme64 left a comment

SirYwell commented Apr 2, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 2, 2025 •

edited

Loading

mlbridge bot commented Apr 2, 2025 •

edited

Loading

SirYwell Apr 10, 2025 •

edited

Loading