8270366: C2: Add associative rule to add/sub node #4765

zhengyu123 · 2021-07-13T12:23:27Z

Please review this small patch that add associative rule to add/sub node, that eliminates a multiply instruction.

e.g.

private static int assocInt(int a, int b, int c) {
  return a * c + b * c;
}

x86_64:
Before:

  0x00007fda1506152c:   imul   %ecx,%esi
  0x00007fda1506152f:   imul   %ecx,%edx
  0x00007fda15061532:   mov    %esi,%eax
  0x00007fda15061534:   add    %edx,%eax                    ;*iadd {reexecute=0 rethrow=0 return_oop=0}
                                                            ; - TestAssoc::assocInt@6 (line 9)

After:

  0x00007fc1c078d52c:   add    %edx,%esi
  0x00007fc1c078d52e:   mov    %ecx,%eax
  0x00007fc1c078d530:   imul   %esi,%eax                    ;*iadd {reexecute=0 rethrow=0 return_oop=0}
                                                            ; - TestAssoc::assocInt@6 (line 9)

Progress

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

Issue

JDK-8270366: C2: Add associative rule to add/sub node

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Andrew Dinn (@adinn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4765

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

Using diff file

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

bridgekeeper · 2021-07-13T12:24:30Z

👋 Welcome back zgu! 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-07-13T12:27:34Z

@zhengyu123 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-07-13T12:31:13Z

Webrevs

adinn · 2021-07-13T14:04:45Z

Hi Zhengyu,

It looks like you are missing a case for both long and int. Your code looks for these patterns:

a*b+a*c --> a*(b+c)
a*b+b*c --> b*(a+c)
a*c+b*c --> (a+b)*c

So you are not transforming this this case

a*b+c*a -> a*(b+c)

dean-long · 2021-07-13T22:47:11Z

Can this rule cause the JIT to compute different results than the interpreter if there is integer overflow? Does the spec allow that?

vnkozlov · 2021-07-14T03:43:34Z

@jddarcy may know the answer.

adinn · 2021-07-14T09:22:32Z

Can this rule cause the JIT to compute different results than the interpreter if there is integer overflow?

I don't believe so.

It should obvious that unsigned arithmetic will always give the same result for any order of computation. Why? Well, if we compute the result for a sequence of n-bit operations in an unlimited bit field we can be sure the results are order independent because of the associativity law. Setting and clearing of higher bits because of overflow beyond n-bits will not change the result that appears in the low n bits.

For signed arithmetic all we need to note is that for every signed n-bit operation that overflows (or underflows) there is an equivalent unsigned operation in 2n bits that has the same bottom n-bits plus some extra non-zero bits in the top n bits. So, for any chain of k signed operations there will be an equivalent chain of unsigned operations in at most kn bits which has the same low n bits. Re-ordering the unsigned operations would produce the same unsigned result which guarantees that an equivalent re-ordering of the signed operations will produce the same signed value in the low n bits.

zhengyu123 · 2021-07-14T14:01:19Z

Hi Zhengyu,

It looks like you are missing a case for both long and int. Your code looks for these patterns:
a*b+a*c --> a*(b+c)
a*b+b*c --> b*(a+c)
a*c+b*c --> (a+b)*c
So you are not transforming this this case
a*b+c*a -> a*(b+c)

zhengyu123 · 2021-07-14T14:02:09Z

Hi Zhengyu,

It looks like you are missing a case for both long and int. Your code looks for these patterns:
a*b+a*c --> a*(b+c)
a*b+b*c --> b*(a+c)
a*c+b*c --> (a+b)*c
So you are not transforming this this case
a*b+c*a -> a*(b+c)

Thanks, Andrew.

Updated.

zhengyu123 · 2021-07-14T14:12:39Z

Can this rule cause the JIT to compute different results than the interpreter if there is integer overflow? Does the spec allow that?

Based on [https://www.agner.org/optimize/optimizing_cpp.pdf](Optimizing software in C++), this optimization is implemented in all 4 compilers (GNU, Clang, Microsoft and Intel) author investigated.

For example:

long mask(long a, long b, long c) {
    return a*b + a*c ;
}

GNU/Clang:

 lea    rax,[rdx+rsi*1]
 imul   rax,rdi

MSVC:

lea     eax, DWORD PTR [rdx+r8]
imul    eax, ecx

vnkozlov · 2021-07-14T15:19:56Z

https://docs.oracle.com/javase/specs/jls/se16/html/jls-15.html#jls-15.17.1

If an integer multiplication overflows, then the result is the low-order bits of the mathematical product as represented in some sufficiently large two's-complement format. As a result, if overflow occurs, then the sign of the result may not be the same as the sign of the mathematical product of the two operand values.

I am interpreting this as using long arithmetic for integer overflow and taking low 32-bits of result. In this sense suggested optimization is legal.

But we may have bugs in VM. I suggest to add a regression test to compare golden (from Interpreter) values with results from optimized compiled code for all interesting/corner cases.

chhagedorn · 2021-07-14T15:55:11Z

This RFE would also be well suited for using the new IR framework to create an IR test to verify the new IR transformations.

zhengyu123 · 2021-07-14T17:41:51Z

https://docs.oracle.com/javase/specs/jls/se16/html/jls-15.html#jls-15.17.1
If an integer multiplication overflows, then the result is the low-order bits of the mathematical product as represented in some sufficiently large two's-complement format. As a result, if overflow occurs, then the sign of the result may not be the same as the sign of the mathematical product of the two operand values.
I am interpreting this as using long arithmetic for integer overflow and taking low 32-bits of result. In this sense suggested optimization is legal.

But we may have bugs in VM. I suggest to add a regression test to compare golden (from Interpreter) values with results from optimized compiled code for all interesting/corner cases.

@vnkozlov Thanks for confirmation.

I added a test as you suggested, but not sure it covers all interesting/corner cases, please advise.

vnkozlov

Repetitive copy-past code pattern concern me - prone to bugs. Can you consider using next pattern instead:

   if (op1 == Op_MulI && op2 == Op_MulI) {
     Node* add_in1 = NULL;
     Node* add_in2 = NULL;
     Node* mul_in = NULL;
     if (in1->in(1) == in2->in(1)) {
       // Convert "a*b+a*c into a *(b+c)
       add_in1 = in1->in(2);
       add_in2 = in2->in(2);
       mul_in = in1->in(1);
...
     if (mul_in != NULL) {
       Node* add = phase->transform(new AddINode(add_in1, add_in2));
       return new MulINode(mul_in, add);
     }

vnkozlov

I am fine with your selection of corner cases in the test.
You need second test file for long values.

vnkozlov · 2021-07-14T17:52:59Z

test/hotspot/jtreg/compiler/integerArithmetic/TestIntegerAssociative.java

+    // Method should be compiled
+    private static int addComp(int a, int b, int c) {
+        return a * b + a * c;
+    }


You don't need separate method for compilation. First call will be executed in interpreter.

vnkozlov · 2021-07-14T17:53:46Z

test/hotspot/jtreg/compiler/integerArithmetic/TestIntegerAssociative.java

+    private static int addInt(int a, int b, int c) {
+        return a * b + a * c;


You need tests for all order variations you optimized.

vnkozlov

Good.

You need second review.

openjdk · 2021-07-14T23:43:10Z

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

8270366: C2: Add associative rule to add/sub node

Reviewed-by: kvn, adinn

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

c962e6e: 8261006: 'super' qualified method references cannot occur in a static context
99d7f9a: 8264908: Investigate adding BOT range check in G1BlockOffsetTablePart::block_at_or_preceding
e92e2fd: 8270517: Add Zero support for LoongArch
7a89ffe: 8270014: Add scoped objects for g1 young gc verification and young gc internal timing
793d772: 8270475: Remove unused G1STWDrainQueueClosure
1ebd946: 8270333: -XX:+VerifyStringTableAtExit should not do linear search
04b73bc: 8269656: The test test/langtools/tools/javac/versions/Versions.java has duplicate test cycles
7c23491: 8269598: Regressions up to 5% on aarch64 seems due to JDK-8268858
7d0edb5: Merge
7b4d84c: 8270422: Test build/AbsPathsInImage.java fails after JDK-8259848
... and 29 more: https://git.openjdk.java.net/jdk/compare/375fc2a2b29c454b36d3ae068a080b28f6ec04e9...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.

adinn

This is definitely clearer and the test is a good safety net.

zhengyu123 · 2021-07-15T16:39:34Z

@adinn @vnkozlov Thanks for the review

zhengyu123 · 2021-07-15T16:39:52Z

/integrate

openjdk · 2021-07-15T16:40:44Z

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

1f995e5: 8265888: StandardJavaFileManager::setLocationForModule specification misses 'Implementation Requirements:'
c962e6e: 8261006: 'super' qualified method references cannot occur in a static context
99d7f9a: 8264908: Investigate adding BOT range check in G1BlockOffsetTablePart::block_at_or_preceding
e92e2fd: 8270517: Add Zero support for LoongArch
7a89ffe: 8270014: Add scoped objects for g1 young gc verification and young gc internal timing
793d772: 8270475: Remove unused G1STWDrainQueueClosure
1ebd946: 8270333: -XX:+VerifyStringTableAtExit should not do linear search
04b73bc: 8269656: The test test/langtools/tools/javac/versions/Versions.java has duplicate test cycles
7c23491: 8269598: Regressions up to 5% on aarch64 seems due to JDK-8268858
7d0edb5: Merge
... and 30 more: https://git.openjdk.java.net/jdk/compare/375fc2a2b29c454b36d3ae068a080b28f6ec04e9...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-07-15T16:40:53Z

@zhengyu123 Pushed as commit 746fe5d.

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

Zhengyu Gu added 2 commits July 11, 2021 10:22

add

c659b3b

sub

3bd7e3b

openjdk bot added the rfr Pull request is ready for review label Jul 13, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 13, 2021

Zhengyu Gu added 2 commits July 14, 2021 09:33

Merge branch 'master' into JDK-8270366-associative

dc5d2ce

Missing case pointed out by Andrew Dinn

83209be

zhengyu123 closed this Jul 14, 2021

zhengyu123 reopened this Jul 14, 2021

Add a test

f95996a

vnkozlov reviewed Jul 14, 2021

View reviewed changes

Zhengyu Gu added 2 commits July 14, 2021 17:30

@vnkozlov's suggestion

14cca14

Complete test

32f942e

vnkozlov approved these changes Jul 14, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 14, 2021

adinn approved these changes Jul 15, 2021

View reviewed changes

openjdk bot closed this Jul 15, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 15, 2021

8270366: C2: Add associative rule to add/sub node #4765

8270366: C2: Add associative rule to add/sub node #4765

zhengyu123 commented Jul 13, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jul 13, 2021

openjdk bot commented Jul 13, 2021

mlbridge bot commented Jul 13, 2021 •

edited

adinn commented Jul 13, 2021 •

edited

dean-long commented Jul 13, 2021

vnkozlov commented Jul 14, 2021

adinn commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

vnkozlov commented Jul 14, 2021

chhagedorn commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

vnkozlov left a comment

vnkozlov left a comment

vnkozlov Jul 14, 2021

vnkozlov Jul 14, 2021

vnkozlov left a comment

openjdk bot commented Jul 14, 2021 •

edited

adinn left a comment

zhengyu123 commented Jul 15, 2021

zhengyu123 commented Jul 15, 2021

openjdk bot commented Jul 15, 2021

openjdk bot commented Jul 15, 2021

		private static int addInt(int a, int b, int c) {
		return a * b + a * c;

8270366: C2: Add associative rule to add/sub node #4765

8270366: C2: Add associative rule to add/sub node #4765

Conversation

zhengyu123 commented Jul 13, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 13, 2021

openjdk bot commented Jul 13, 2021

mlbridge bot commented Jul 13, 2021 • edited

Webrevs

adinn commented Jul 13, 2021 • edited

dean-long commented Jul 13, 2021

vnkozlov commented Jul 14, 2021

adinn commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

vnkozlov commented Jul 14, 2021

chhagedorn commented Jul 14, 2021

zhengyu123 commented Jul 14, 2021

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov Jul 14, 2021

Choose a reason for hiding this comment

vnkozlov Jul 14, 2021

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 14, 2021 • edited

adinn left a comment

Choose a reason for hiding this comment

zhengyu123 commented Jul 15, 2021

zhengyu123 commented Jul 15, 2021

openjdk bot commented Jul 15, 2021

openjdk bot commented Jul 15, 2021

zhengyu123 commented Jul 13, 2021 •

edited by openjdk bot

mlbridge bot commented Jul 13, 2021 •

edited

adinn commented Jul 13, 2021 •

edited

openjdk bot commented Jul 14, 2021 •

edited