8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x". #6858

CptGit · 2021-12-16T00:02:12Z

Hi all,

Existing optimization ~x+1 -> -x can be generalized to ~x+c -> (c-1)-x. I included both microbenchmark and jtreg tests.

// Convert (~x+c) into (c-1)-x. Note there isn't a bitwise not
// bytecode, "~x" would typically represented as "x^(-1)", so (~x+c)
// will be (x^(-1))+c.
if (op1 == Op_Xor(bt) &&
    (in2->Opcode() == Op_ConI || in2->Opcode() == Op_ConL) &&
    phase->type(in1->in(2)) == TypeInteger::minus_1(bt)) {
  Node* c_minus_one = phase->makecon(add_ring(phase->type(in(2)), TypeInteger::minus_1(bt)));
  return SubNode::make(c_minus_one, in1->in(1), bt);
}

Thank you for reviewing.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x".

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6858/head:pull/6858
$ git checkout pull/6858

Update a local copy of the PR:
$ git checkout pull/6858
$ git pull https://git.openjdk.java.net/jdk pull/6858/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6858

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6858.diff

Because java_subtract cannot tell int from long in this case.

bridgekeeper · 2021-12-16T00:02:56Z

👋 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 · 2021-12-16T00:05:35Z

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

…PAddSub1202

CptGit · 2021-12-19T22:05:53Z

Attach benchmark results:

Baseline:

Benchmark                       Mode  Cnt  Score   Error  Units
AddIdealNotXPlusC.baselineInt   avgt   60  1.479 ± 0.004  ns/op
AddIdealNotXPlusC.baselineLong  avgt   60  1.479 ± 0.006  ns/op
AddIdealNotXPlusC.testInt       avgt   60  3.142 ± 0.019  ns/op
AddIdealNotXPlusC.testLong      avgt   60  3.150 ± 0.019  ns/op

Patch:

Benchmark                       Mode  Cnt  Score   Error  Units
AddIdealNotXPlusC.baselineInt   avgt   60  1.489 ± 0.008  ns/op
AddIdealNotXPlusC.baselineLong  avgt   60  1.488 ± 0.007  ns/op
AddIdealNotXPlusC.testInt       avgt   60  0.343 ± 0.001  ns/op
AddIdealNotXPlusC.testLong      avgt   60  0.343 ± 0.001  ns/op

mlbridge · 2022-01-07T17:28:53Z

Webrevs

TobiHartmann

Isn't the same applicable to the corresponding XoINode and XorLNode optimizations?

jdk/src/hotspot/share/opto/addnode.cpp

Lines 877 to 883 in 126328c

    
           // Convert ~(x-1) into -x. Note there isn't a bitwise not bytecode, 
        
           // "~x" would typically represented as "x^(-1)", and "x-c0" would 
        
           // convert into "x+ -c0" in SubXNode::Ideal. So ~(x-1) will eventually 
        
           // be (x+(-1))^-1. 
        
           if (op1 == Op_AddI && phase->type(in2) == TypeInt::MINUS_1 && 
        
               phase->type(in1->in(2)) == TypeInt::MINUS_1) { 
        
             return new SubINode(phase->makecon(TypeInt::ZERO), in1->in(1));

CptGit · 2022-01-15T18:37:29Z

Isn't the same applicable to the corresponding XoINode and XorLNode optimizations?

jdk/src/hotspot/share/opto/addnode.cpp

Lines 877 to 883 in 126328c

// Convert ~(x-1) into -x. Note there isn't a bitwise not bytecode,

// "~x" would typically represented as "x^(-1)", and "x-c0" would

// convert into "x+ -c0" in SubXNode::Ideal. So ~(x-1) will eventually

// be (x+(-1))^-1.

if (op1 == Op_AddI && phase->type(in2) == TypeInt::MINUS_1 &&

phase->type(in1->in(2)) == TypeInt::MINUS_1) {

return new SubINode(phase->makecon(TypeInt::ZERO), in1->in(1));

Great catch, thanks. I included similar transformation for XorINode and XorLNode.

TobiHartmann

That looks good to me and all testing passed.

You need a second review.

TobiHartmann · 2022-01-19T08:43:47Z

/reviewers 2

openjdk · 2022-01-19T08:46:29Z

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

8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x".

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

84fa0d8: 8190264: JScrollBar ignores its border when using macOS Mac OS X Aqua look and feel
610a129: 8268831: Improve javadoc tool handling of streams.
e20c6bf: 8280189: JFR: TestPrintXML should print mismatching XML
b20b11c: 8258240: make vscode-project on Windows generates jdk.code-workspace file with unescaped '' in paths
9611431: 8279936: Change shared code to use os:: system API's
cc2f474: 8280024: Parallel: Remove unnecessary region resizing methods in PSCardTable
8931c12: 8280157: wrong texts Falied in a couple of tests
68b40ec: 8273139: C2: assert(f <= 1 && f >= 0) failed: Incorrect frequency
39b1d75: 8277822: Remove debug-only heap overrun checks in os::malloc and friends
5af7f25: 8274811: Remove superfluous use of boxing in java.base
... and 145 more: https://git.openjdk.java.net/jdk/compare/4243f4c998344e77dccd4d5605e56e869bc8af89...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 (@TobiHartmann, @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).

openjdk · 2022-01-19T08:46:50Z

@TobiHartmann
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

CptGit · 2022-01-19T17:43:04Z

That looks good to me and all testing passed.

You need a second review.

Thanks for reviewing.

vnkozlov

Good.

CptGit · 2022-01-19T18:59:16Z

/integrate

openjdk · 2022-01-19T19:11:43Z

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

TobiHartmann · 2022-01-20T07:36:21Z

/sponsor

openjdk · 2022-01-20T07:38:13Z

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

e683d4a: 8279921: Dump the .class file in jlink debug mode for any failure during transform() of a plugin
1022cbd: 8280047: Broken link to Swing Connection document from javax.swing package docs
c4a624d: 8279894: javax/swing/JInternalFrame/8020708/bug8020708.java timeouts on Windows 11
4616c13: Merge
03680be: 8280233: Temporarily disable Unix domain sockets in Windows PipeImpl
be0538d: 8278834: Error "Cannot read field "sym" because "this.lvar[od]" is null" when compiling
f5de6fa: 8272058: 25 Null pointer dereference defect groups in 4 files
28e02fa: 8280234: AArch64 "core" variant does not build after JDK-8270947
f37bfea: 8280155: [PPC64, s390] frame size checks are not yet correct
69cfa9c: 8273383: vmTestbase/vm/gc/containers/Combination05/TestDescription.java crashes verifying length of DCQS
... and 193 more: https://git.openjdk.java.net/jdk/compare/4243f4c998344e77dccd4d5605e56e869bc8af89...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-20T07:38:33Z

@TobiHartmann @CptGit Pushed as commit 0bf95a1.

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

CptGit added 5 commits December 14, 2021 17:09

generalize transformation from (~x+1) into -x.

37c1d87

Use add_ring instead of java_subtract to compute for constants.

430badf

Because java_subtract cannot tell int from long in this case.

Include a jtreg test for the optimization introduced.

705e064

Merge master.

539ffa0

add the associated microbenchmark test.

d1b9d37

openjdk bot added the hotspot-compiler [email protected] label Dec 16, 2021

CptGit changed the title ~~Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x"~~ [TBD]: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x" Dec 16, 2021

icrease the number of iterations in microbenmark tests to reduce errors.

d32a18e

CptGit marked this pull request as draft December 18, 2021 03:26

CptGit added 3 commits December 18, 2021 16:28

replace normal jtreg test with ir test.

b549b27

update microbenchmark.

725be3a

Merge branch 'master' of https://github.com/openjdk/jdk into addnode-…

3e0c914

…PAddSub1202

CptGit marked this pull request as ready for review December 19, 2021 00:37

update benchmark.

a2daf3f

CptGit added 4 commits January 4, 2022 14:50

update copyright year.

23dcb06

fix copyright year.

aa80aa0

Merge branch 'master'.

ccbb3b9

CptGit changed the title ~~[TBD]: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x"~~ 8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x" Jan 7, 2022

openjdk bot changed the title ~~8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x"~~ 8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x". Jan 7, 2022

openjdk bot added the rfr Pull request is ready for review label Jan 7, 2022

include bug id for the test.

05bdf28

Merge branch 'master'.

5c9bcfd

TobiHartmann reviewed Jan 11, 2022

View reviewed changes

transform ~(x+c) to (-c-1)-x.

c540be3

cast to jlong instead of long.

4285a23

TobiHartmann approved these changes Jan 19, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 19, 2022

openjdk bot removed the ready Pull request is ready to be integrated label Jan 19, 2022

vnkozlov approved these changes Jan 19, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 19, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 19, 2022

openjdk bot added the integrated Pull request has been integrated label Jan 20, 2022

openjdk bot closed this Jan 20, 2022

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 Jan 20, 2022

CptGit mentioned this pull request Feb 8, 2022

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

Closed

3 tasks

	// Convert ~(x-1) into -x. Note there isn't a bitwise not bytecode,
	// "~x" would typically represented as "x^(-1)", and "x-c0" would
	// convert into "x+ -c0" in SubXNode::Ideal. So ~(x-1) will eventually
	// be (x+(-1))^-1.
	if (op1 == Op_AddI && phase->type(in2) == TypeInt::MINUS_1 &&
	phase->type(in1->in(2)) == TypeInt::MINUS_1) {
	return new SubINode(phase->makecon(TypeInt::ZERO), in1->in(1));

8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x". #6858

8279607: Existing optimization "~x+1" -> "-x" can be generalized to "~x+c" -> "(c-1)-x". #6858

Uh oh!

Conversation

CptGit commented Dec 16, 2021 • 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 Dec 16, 2021

Uh oh!

openjdk bot commented Dec 16, 2021

Uh oh!

CptGit commented Dec 19, 2021

Uh oh!

mlbridge bot commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment • 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!

CptGit commented Jan 15, 2022

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jan 19, 2022

Uh oh!

openjdk bot commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jan 19, 2022

Uh oh!

CptGit commented Jan 19, 2022

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

CptGit commented Jan 19, 2022

Uh oh!

openjdk bot commented Jan 19, 2022

Uh oh!

TobiHartmann commented Jan 20, 2022

Uh oh!

openjdk bot commented Jan 20, 2022

Uh oh!

openjdk bot commented Jan 20, 2022

Uh oh!

Uh oh!

CptGit commented Dec 16, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 7, 2022 •

edited

Loading

TobiHartmann left a comment •

edited

Loading

openjdk bot commented Jan 19, 2022 •

edited

Loading