8281453: New optimization: convert ~x into -1-x when ~x is used in an arithmetic expression

[CptGit]

Similar to (~x)+c -> (c-1)-x and ~(x+c) -> (-c-1)-x in #6858, we can also introduce similar optimizations for subtraction, c-(~x) -> x+(c+1) and ~(c-x) -> x+(-c-1).

To generalize, I convert ~x into -1-x when ~x is used only in arithmetic expression. For example, c-(~x) will be converted into c-(-1-x) which will match other pattern and will be transformed again in next iteration and finally become x+(c+1).

Also the conversion from ~x into -1-x happens when x is an arithmetic expression itself. For example, ~(x+c) will be transformed into -1-(x+c) and eventually (-c-1)-x.

The results of the microbenchmark are as follows:

Baseline:                                                               
Benchmark                         Mode  Cnt  Score   Error  Units
NotOpTransformation.baselineInt   avgt   60  0.440 ± 0.002  ns/op
NotOpTransformation.baselineLong  avgt   60  0.440 ± 0.001  ns/op
NotOpTransformation.testInt1      avgt   60  0.613 ± 0.006  ns/op
NotOpTransformation.testInt2      avgt   60  0.868 ± 0.036  ns/op
NotOpTransformation.testLong1     avgt   60  0.674 ± 0.008  ns/op
NotOpTransformation.testLong2     avgt   60  0.698 ± 0.006  ns/op

Patch:
Benchmark                         Mode  Cnt  Score    Error  Units
NotOpTransformation.baselineInt   avgt   60  0.440 ±  0.001  ns/op
NotOpTransformation.baselineLong  avgt   60  0.440 ±  0.001  ns/op
NotOpTransformation.testInt1      avgt   60  0.329 ±  0.001  ns/op
NotOpTransformation.testInt2      avgt   60  0.329 ±  0.001  ns/op
NotOpTransformation.testLong1     avgt   60  0.329 ±  0.001  ns/op
NotOpTransformation.testLong2     avgt   60  0.329 ±  0.001  ns/op

---

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-8281453: New optimization: convert ~x into -1-x when ~x is used in an arithmetic expression

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7376

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

Using diff file

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

[bridgekeeper]

👋 Welcome back CptGit! 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]

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

[TobiHartmann]

A bug has been filed https://bugs.openjdk.java.net/browse/JDK-8281453.

[mlbridge]

Webrevs

• 14: Full - Incremental (91407ddf)
• 13: Full - Incremental (d7183474)
• 12: Full (bb904b5b)
• 11: Full - Incremental (d9582136)
• 10: Full - Incremental (26274704)
• 09: Full - Incremental (ec3360d4)
• 08: Full - Incremental (1c238b8d)
• 07: Full (f7345fd9)
• 06: Full (1a97ba26)
• 05: Full - Incremental (666be2d8)
• 04: Full - Incremental (3f0d6a50)
• 03: Full - Incremental (5f1939c9)
• 02: Full - Incremental (51c98b5f)
• 01: Full - Incremental (76bde38e)
• 00: Full (35abcb2d)

[merykitty]

Since ~x == -1 - x and these 2 operations' costs are essentially the same. It would be much easier if you just check whether the not result is used in an arithmetic operation and transform the former to the latter. The reverse is also true, if you find a -1 - x being fed into a bitwise just transform it to a ~x then.
Thanks.

[bridgekeeper]

@CptGit This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[CptGit]

Since ~x == -1 - x and these 2 operations' costs are essentially the same. It would be much easier if you just check whether the not result is used in an arithmetic operation and transform the former to the latter. The reverse is also true, if you find a -1 - x being fed into a bitwise just transform it to a ~x then. Thanks.

Thank you for your comment. I am looking at this pending PR again. I drafted it for now until I get a rfr version.

While I am implementing your suggestion, I found another typical optimization that is missing, which is pushing constant to right from x + (con - y) to (x - y) + con. I created a separate PR for it, #7795. Can you take a look at that one if you get a chance? Thank you!

[CptGit]

Hello @merykitty I finished converting ~x to -1-x when it appears in any add or sub nodes, as you suggested; so now it can really support a wide list of transformation: c-(~x) => x + (c+1), ~x - ~y => y - x, (x+1) + ~y => x - y, etc, which are all created as new tests. Can you help review again thank you.

[vnkozlov]

This change touches the same code as #7795
I suggest to update after that one is pushed.

[CptGit]

This change touches the same code as #7795 I suggest to update after that one is pushed.

Merged master after #7795 pushed. Can you look at the updated change? Thank you. @vnkozlov

[vnkozlov]

Optimization you proposed does not match RFE description and title.

You do only: ~x or (x ^ (-1))->(-1 - x)`

As result this should be Xor nodes ideal transformation. I don't even think you need such transformation if rhs and lhs are not constants because I assume XOR and SUB hw instructions have the same latency.

I suggest you to redo performance testing after you merged #7795 changes.

[CptGit]

Thanks for reviewing. @vnkozlov

Optimization you proposed does not match RFE description and title.

The initial description is somewhat out of date because I adopted @merykitty 's suggestion to include all the cases enabled from ~x => -1-x instead of adding them one by one, such as (~x)+c -> (c-1)-x, which exist in current code base, and many new idealisation enabled from ~x => -1-x such as c-(~x) => x + (c+1), ~x - ~y => y - x, (x+1) + ~y => x - y I did not update description and title because I'd like to keep history somehow. Please let me know if I should.

I suggest you to redo performance testing after you merged #7795 changes.

Will redo and post results.

[bridgekeeper]

@CptGit This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@CptGit This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[CptGit]

@merykitty I moved things to XorINode and XorLNode and made generalization. Can you please have a look when you get a chance if my changes make sense? Thank you.

[CptGit]

@merykitty I removed the use check. Does it look good to you? I did not include test (x + y) & ~(x + y) => 0 because I found we do not have such idealization in AndINode because even x & ~x => 0 is not supported.

[merykitty]

I think it looks good, thanks.

[vnkozlov]

I will test latest version.

[vnkozlov]

Latest testing passed.

[vnkozlov]

@CptGit Did you updated performance numbers for latest changes? Did they change?

[CptGit]

@CptGit Did you updated performance numbers for latest changes? Did they change?

@vnkozlov Updated now. Thanks.

[vnkozlov]

Good. It is ready for integration.

[CptGit]

/integrate

[openjdk]

@CptGit This pull request has not yet been marked as ready for integration.

[vnkozlov]

@CptGit This pull request has not yet been marked as ready for integration.

I think, it is because PR title does not match JBS entry title.

[openjdk]

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

8281453: New optimization: convert ~x into -1-x when ~x is used in an arithmetic expression

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

• 85569e9: 8286037: Bump minimum boot jdk to JDK 19
• fef345b: 8293672: Update freetype md file
• 5c030cc: 8290973: In AffineTransform, equals(Object) is inconsistent with hashCode()
• f888aa9: 8293061: Combine CDSOptions and AppCDSOptions test utility classes
• 73f0646: 8294839: Disable StressLongCountedLoop in compiler/loopopts/TestRemoveEmptyLoop.java
• 2ceebf6: 8294456: Fix misleading-indentation warnings in core JDK libraries
• ad7b7d4: 8294697: java/lang/Thread/virtual/ThreadAPI.testGetStackTrace2 failed with non-empty stack trace
• e38ae8a: 8294759: Print actual lock/monitor ranking
• 7012d4b: 8294837: unify Windows 2019 version check in os_windows and java_props_md
• 8c15f77: 8270915: GIFImageReader disregards ignoreMetadata flag which causes memory exhaustion
• ... and 81 more: https://git.openjdk.org/jdk/compare/aeef3ecdc4d99d4bfb9e762cb038d9571c3c56df...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 (@vnkozlov) 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).

[CptGit]

/integrate

[openjdk]

@CptGit
Your change (at version 91407dd) is now ready to be sponsored by a Committer.

[vnkozlov]

/sponsor

[openjdk]

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

• 85569e9: 8286037: Bump minimum boot jdk to JDK 19
• fef345b: 8293672: Update freetype md file
• 5c030cc: 8290973: In AffineTransform, equals(Object) is inconsistent with hashCode()
• f888aa9: 8293061: Combine CDSOptions and AppCDSOptions test utility classes
• 73f0646: 8294839: Disable StressLongCountedLoop in compiler/loopopts/TestRemoveEmptyLoop.java
• 2ceebf6: 8294456: Fix misleading-indentation warnings in core JDK libraries
• ad7b7d4: 8294697: java/lang/Thread/virtual/ThreadAPI.testGetStackTrace2 failed with non-empty stack trace
• e38ae8a: 8294759: Print actual lock/monitor ranking
• 7012d4b: 8294837: unify Windows 2019 version check in os_windows and java_props_md
• 8c15f77: 8270915: GIFImageReader disregards ignoreMetadata flag which causes memory exhaustion
• ... and 81 more: https://git.openjdk.org/jdk/compare/aeef3ecdc4d99d4bfb9e762cb038d9571c3c56df...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vnkozlov @CptGit Pushed as commit 5dd851d.

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