8261008: Optimize Xor #2776

e1iu · 2021-03-01T09:36:37Z

This patch canonicalizes 'Xor' to constant zero when both inputs are the
same.

It's not quite easy to measure the performance change between 'xor' and
constant zero, the later is typically a single 'mov' in generated code.
But given by this transformation, c2 may perform some other more
powerful optimizations.

This was tested with the micro benchmark below. Loop in this case is
properly removed and the performance increases significantly.

public void xorTheSame(MyState s, Blackhole bh) {
  int x = s.in1;
  int y = s.in2;
  for (int i = 0; i < 5000; i++) {
    y = x ^ x;
    x = y ^ y;
  }
  bh.consume(x);
}

[Test]
All jtreg tests passed without new failure.

Progress

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

Issue

JDK-8261008: Optimize Xor

Reviewers

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

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2776/head:pull/2776
$ git checkout pull/2776

This patch canonicalizes 'Xor' to constant zero when both inputs are the same. It's not quite easy to measure the performance change between 'xor' and constant zero, the later is typically a single 'mov' in generated code. But given by this transformation, c2 may perform some other more powerful optimizations. This was tested with the micro benchmark below. Loop in this case is properly removed and the performance increases significantly. ``` public void xorTheSame(MyState s, Blackhole bh) { int x = s.in1; int y = s.in2; for (int i = 0; i < 5000; i++) { y = x ^ x; x = y ^ y; } bh.consume(x); } ``` [Test] All jtreg tests passed without new failure. Change-Id: I1334199868b07543c4fe004976c26bed9162a993

bridgekeeper · 2021-03-01T09:37:08Z

👋 Welcome back theRealELiu! 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-03-01T09:38:20Z

@theRealELiu 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 · 2021-03-01T09:42:01Z

Webrevs

01: Full - Incremental (fd25d5b)
00: Full (7e1588c)

TobiHartmann

Just wondering, why is x ^ x special? Shouldn't we then also optimize x - x, x | x and others?

bernardosulzbach · 2021-03-01T12:56:24Z

If we agree that this optimization is desired, then it makes sense to also write optimizations for x - x and x | x.

However, how often do these patterns occur in real code? Can you provide some examples (of source code from OSS projects) where x ^ x, x - x, or x | x occur?

e1iu · 2021-03-01T13:25:50Z

Just wondering, why is x ^ x special? Shouldn't we then also optimize x - x, x | x and others?

I found this case by accident and didn't notice that other node might share the same problem. I suppose they would also bring some benifits without much effort.

e1iu · 2021-03-01T14:24:14Z

If we agree that this optimization is desired, then it makes sense to also write optimizations for x - x and x | x.

However, how often do these patterns occur in real code? Can you provide some examples (of source code from OSS projects) where x ^ x, x - x, or x | x occur?

Thanks for your feedback. I didn't find them in real cases. This case was discussed in https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-November/041798.html.

To persue the optimal code, it could be done without too much effort.

vnkozlov · 2021-03-01T20:42:49Z

x - x is already optimized: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/subnode.cpp#L96
x | x - here: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/addnode.cpp#L781

For x ^ x Value() method should be used as for SubNode.

Change-Id: Ic9fc01375801adc82c0a0289d9a11a5367031eb4

e1iu · 2021-03-02T06:38:28Z

If we agree that this optimization is desired, then it makes sense to also write optimizations for x - x and x | x.

However, how often do these patterns occur in real code? Can you provide some examples (of source code from OSS projects) where x ^ x, x - x, or x | x occur?

Hi Bernardo,

I tried to find this by adding some instruments.

The JDK itself exactly has this pattern.

Current thread (0x0000fffedc3f9130):  JavaThread "C2 CompilerThread3" daemon [_thread_in_native, id=3972, stack(0x0000fffecfa00000,0x0000fffecfc00000)]


 Current CompileTask:
 C2:   2687 1278       4       jdk.tools.jlink.internal.ImageFileCreator$$Lambda$127/0x00000008000ed3b0::accept (15 bytes)

 Stack: [0x0000fffecfa00000,0x0000fffecfc00000],  sp=0x0000fffecfbf9d70,  free space=2023k
 Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
 V  [libjvm.so+0x5eb314]  XorINode::Value(PhaseGVN*) const+0x134
 V  [libjvm.so+0x128a5b0]  PhaseIdealLoop::split_thru_phi(Node*, Node*, int)+0x1d0
 V  [libjvm.so+0x128d7a4]  PhaseIdealLoop::split_if_with_blocks_pre(Node*)+0x2b4
 V  [libjvm.so+0x128f360]  PhaseIdealLoop::split_if_with_blocks(VectorSet&, Node_Stack&)+0x160
 V  [libjvm.so+0x1283280]  PhaseIdealLoop::build_and_optimize(LoopOptsMode)+0xf50
 V  [libjvm.so+0x9cfadc]  PhaseIdealLoop::optimize(PhaseIterGVN&, LoopOptsMode)+0x28c
 V  [libjvm.so+0x9cd21c]  Compile::Optimize()+0xf3c
 V  [libjvm.so+0x9ce580]  Compile::Compile(ciEnv*, ciMethod*, int, bool, bool, bool, bool, DirectiveSet*)+0xf50
 V  [libjvm.so+0x80ddbc]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x178
 V  [libjvm.so+0x9de684]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x9d0
 V  [libjvm.so+0x9df22c]  CompileBroker::compiler_thread_loop()+0x328
 V  [libjvm.so+0x18bac90]  JavaThread::thread_main_inner()+0x37c
 V  [libjvm.so+0x18c1a78]  Thread::call_run()+0xf8
 V  [libjvm.so+0x149ac60]  thread_native_entry(Thread*)+0x120
 C  [libpthread.so.0+0x7088]  start_thread+0xb0

I think no one would write those kind of code directly, but GVN, after all, was a general phase which may create new node during several iteration. Besides the inline method may lead to this as well.

--Eric

bernardosulzbach · 2021-03-02T07:50:28Z

@theRealELiu that's a very interesting find!

It makes me wonder if these aren't also warnings analysis tools should provide (if it's caused by optimizations of user code), something like "expression is always 0".

e1iu · 2021-03-02T10:54:53Z

x - x is already optimized: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/subnode.cpp#L96
x | x - here: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/addnode.cpp#L781

For x ^ x Value() method should be used as for SubNode.

Thanks for your review. I moved those codes into Value() and will update after some tests finished.

TobiHartmann · 2021-03-03T14:32:45Z

I think C2 optimizations could lead to IR shapes containing that pattern even if the Java source code does not. In any case, such simple patterns should be optimized and as Vladimir mentioned, it should be a ::Value optimization.

e1iu · 2021-03-04T13:23:59Z

Please help to review the new commit.

All jtreg tests passed without new failure.

vnkozlov

Good.

openjdk · 2021-03-04T19:32:58Z

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

8261008: Optimize Xor

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

d91550e: 8262998: Vector API intrinsincs should not modify IR when bailing out
80182f9: 8260925: HttpsURLConnection does not work with other JSSE provider.
dbef0ec: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
351889f: 8262508: Vector API's ergonomics is incorrect
718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
c8b23e2: 8262064: Make compiler/ciReplay tests ignore lambdas in compilation replay
02fbcb5: 8261532: Archived superinterface class cannot be accessed
109af7b: 8261518: jpackage looks for main module in current dir when there is no module-path
e61a3ba: 8239386: handle ContendedPaddingWidth in vm_version_aarch64
... and 112 more: https://git.openjdk.java.net/jdk/compare/382e38dd246596ec94a1f1ce0e0f9e87f53366c7...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).

TobiHartmann

Looks good to me too.

e1iu · 2021-03-05T08:00:55Z

/integrate

openjdk · 2021-03-05T08:01:30Z

@theRealELiu
Your change (at version fd25d5b) is now ready to be sponsored by a Committer.

vnkozlov · 2021-03-06T03:08:58Z

/sponsor

openjdk · 2021-03-06T08:53:56Z

@vnkozlov @theRealELiu Since your change was applied there have been 129 commits pushed to the master branch:

e1cad97: 8262862: Harden tests sun/security/x509/URICertStore/ExtensionsWithLDAP.java and krb5/canonicalize/Test.java
2c0507e: 8261812: C2 compilation fails with assert(!had_error) failed: bad dominance
9755782: 8157682: @inheritdoc doesn't work with @exception
8c13d26: 8263050: move HtmlDocletWriter.verticalSeparator to IndexWriter
8d3de4b: 8262844: (fs) FileStore.supportsFileAttributeView might return false negative in case of ext3
75fb7cc: 8259228: Zero: rewrite (put|get)field from if-else chains to switches
9730266: 8262973: Verify ParCompactionManager instance in PCAdjustPointerClosure
d91550e: 8262998: Vector API intrinsincs should not modify IR when bailing out
80182f9: 8260925: HttpsURLConnection does not work with other JSSE provider.
dbef0ec: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
... and 119 more: https://git.openjdk.java.net/jdk/compare/382e38dd246596ec94a1f1ce0e0f9e87f53366c7...master

Your commit was automatically rebased without conflicts.

Pushed as commit 23ee60d.

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

openjdk bot added the rfr Pull request is ready for review label Mar 1, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 1, 2021

TobiHartmann reviewed Mar 1, 2021

View reviewed changes

Implements Value() for XorNode.

fd25d5b

Change-Id: Ic9fc01375801adc82c0a0289d9a11a5367031eb4

vnkozlov approved these changes Mar 4, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 4, 2021

TobiHartmann approved these changes Mar 5, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 5, 2021

openjdk bot closed this Mar 6, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 6, 2021

e1iu deleted the xor-ideal branch March 18, 2021 04:11

fg1417 mentioned this pull request Dec 20, 2021

8276673: Optimize abs operations in C2 compiler #6755

Closed

3 tasks

liach mentioned this pull request Jan 14, 2025

8347645: C2: XOR bounded value handling blocks constant folding #23089

Closed

3 tasks

8261008: Optimize Xor #2776

8261008: Optimize Xor #2776

Uh oh!

Conversation

e1iu commented Mar 1, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Mar 1, 2021

Uh oh!

openjdk bot commented Mar 1, 2021

Uh oh!

mlbridge bot commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

bernardosulzbach commented Mar 1, 2021 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

e1iu commented Mar 1, 2021

Uh oh!

e1iu commented Mar 1, 2021

Uh oh!

vnkozlov commented Mar 1, 2021

Uh oh!

e1iu commented Mar 2, 2021

Uh oh!

bernardosulzbach commented Mar 2, 2021

Uh oh!

e1iu commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann commented Mar 3, 2021

Uh oh!

e1iu commented Mar 4, 2021

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

e1iu commented Mar 5, 2021

Uh oh!

openjdk bot commented Mar 5, 2021

Uh oh!

vnkozlov commented Mar 6, 2021

Uh oh!

openjdk bot commented Mar 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

e1iu commented Mar 1, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 1, 2021 •

edited

Loading

bernardosulzbach commented Mar 1, 2021 •

edited by bridgekeeper bot

Loading

e1iu commented Mar 2, 2021 •

edited

Loading

openjdk bot commented Mar 4, 2021 •

edited

Loading