8291336: Add ideal rule to convert floating point multiply by 2 into addition

[jaskarth]

Hello, I would like to propose an ideal transform that converts floating point multiply by 2 (x * 2) into an addition operation instead. This would allow for the elimination of the memory reference for the constant two, and keep the whole operation inside registers. My justifications for this optimization include:

• As per Agner Fog's instruction tables many older systems, such as the sandy bridge and ivy bridge architectures, have different latencies for addition and multiplication meaning this change could have beneficial effects when in hot code.
• The removal of the memory load would have a beneficial effect in cache bound situations.
• Multiplication by 2 is relatively common construct so this change can apply to a wide range of Java code.

As this is my first time looking into the c2 codebase, I have a few lingering questions about my implementation and how certain parts of the compiler work. Mainly, is this patch getting the type of the operands correctly? I saw some cases where code used bottom_type() and other cases where it used phase->type(value). Similarly, are nodes able to be reused as is being done in the AddNode constructors? I saw some places where the clone method was being used, but other places where it wasn't.

I have attached an IR test and a jmh benchmark. Tier 1 testing passes on my machine.

Thanks for your time, Jasmine

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [x] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8291336: Add ideal rule to convert floating point multiply by 2 into addition

Reviewers

• Quan Anh Mai (@merykitty - Committer)
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9642

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back SuperCoder7979! 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[bot]]

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

[jaskarth]

Unfortunately it seems I can't open bugs on the JBS, is there a way to do so or will someone else have to do it for me?

[merykitty]

There you go: https://bugs.openjdk.org/browse/JDK-8291336

Next time you could ask for help in the appropriate mailing list (this time it is hotspot-compiler-dev) or submit a bug through https://bugreport.java.com/bugreport/

Also please enable github action in your fork so that the patches get tested automatically at tier 1 on major platforms.

Hope this helps.

[mlbridge[bot]]

Webrevs

• 04: Full - Incremental (674124d3)
• 03: Full - Incremental (d4303fad)
• 02: Full - Incremental (bce4263c)
• 01: Full - Incremental (04706500)
• 00: Full (1448f25a)

[jaskarth]

Hi, thank you for your assistance with this, I have updated the PR title and have applied the changes from code review. I have also updated the benchmark and have attached the results below. I tested the benchmark on 2 systems, a new one and an old one. The new system has a Ryzen 5 4500U cpu, and the results are as shown:

                                       Baseline                  Patch
Benchmark                 Mode  Cnt    Score   Error  Units      Score   Error  Units
TestMul2.testMul2Double   avgt   10  209.740 ± 1.454  ns/op // 209.315 ± 1.116  ns/op (+0.20%)
TestMul2.testMul2Float    avgt   10  210.871 ± 6.179  ns/op // 209.498 ± 0.777  ns/op (+0.65%)

The benchmark showed very little change on the new system, which is expected as the documentation states that both the vaddsd and vmulsd instructions have a latency of 3 cycles and a reciprocal throughput of 0.5. The slight gain could be from the elimination of the memory reference, or just from testing variance. The older system ran a Xeon x5690, and had these results:

                                       Baseline                  Patch
Benchmark                 Mode  Cnt    Score   Error  Units      Score   Error  Units
TestMul2.testMul2Double   avgt   10  190.062 ± 9.695  ns/op // 170.393 ± 1.193  ns/op (+10.34%)
TestMul2.testMul2Float    avgt   10  184.239 ± 1.983  ns/op // 171.329 ± 4.261  ns/op (+7.00%)

Due to the older system having a faster addition than multiplication, especially with double precision operations, far more substantial gains were realized here.

[jaskarth]

Thank you for testing! I have applied changes from code review.

[TobiHartmann]

/reviewers 2

[openjdk[bot]]

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

8291336: Add ideal rule to convert floating point multiply by 2 into addition

Reviewed-by: qamai, thartmann, chagedorn

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

• 4b89fce0831f990d4b6af5e6e208342f68aed614: 8291781: assert(!is_visited) failed: visit only once with -XX:+SuperWordRTDepCheck
• d5d34241e21305379f1858556f225e7645cd294e: 8295405: Add cause in a couple of IllegalArgumentException and InvalidParameterException shown by sun/security/pkcs11 tests
• fd668dc44f54274518d2bb46c5e22318a872c02e: 8295537: Enhance TRACE_METHOD_LINKAGE to show the target MethodHandle
• 182c215888fa2f58f9d1f4cfb32f1f45012b8d9f: 8295994: Remove left over InetAddressContainer class
• 78763fc8e0d6940f1c85ff8705733b8e6ae8e945: 8295000: java/util/Formatter/Basic test cleanup
• 907d583376dfab269ea25a6c036e390f3484065e: 8295323: Unnecessary HashTable usage in StyleSheet
• 2157145766f9789ade0940e9ae1715a3b74d508b: 8293858: Change PKCS7 code to use default SecureRandom impl instead of SHA1PRNG
• b8ad6cd98a7e4b577b888dc5f9d93c2e4d3bf177: 8294461: wrong effectively final determination by javac
• d6678952a6de4e5435dab65e7029021832454857: 8294399: (ch) Refactor some methods out of sun.nio.ch.UnixFileDispatcherImpl
• 628820f47ef9c9ad3cc62e68db9c4dbc7e659154: 8283093: JMX connections should default to using an ObjectInputFilter
• ... and 1134 more: https://git.openjdk.org/jdk/compare/0ca74f538e1a8a351cc0631c5fe397a74653ce6f...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 (@merykitty, @TobiHartmann, @chhagedorn) 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[bot]]

@TobiHartmann The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[jaskarth]

Hi, may I have more reviews on this change, please? Thanks

[bridgekeeper[bot]]

@SuperCoder7979 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!

[jaskarth]

Hi, apologies for the delayed reply but I have fixed the style and have added verification of the optimization against the interpreter. A re-review would be much appreciated. Thanks for your time once again!

[jaskarth]

Thank you!

[jaskarth]

/integrate

[openjdk[bot]]

@SuperCoder7979 Your change (at version 674124d3548019d17afbcf6b298a13cc40463b94) is now ready to be sponsored by a Committer.

[chhagedorn]

Testing looked good!

/sponsor

[openjdk[bot]]

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

• 4b89fce0831f990d4b6af5e6e208342f68aed614: 8291781: assert(!is_visited) failed: visit only once with -XX:+SuperWordRTDepCheck
• d5d34241e21305379f1858556f225e7645cd294e: 8295405: Add cause in a couple of IllegalArgumentException and InvalidParameterException shown by sun/security/pkcs11 tests
• fd668dc44f54274518d2bb46c5e22318a872c02e: 8295537: Enhance TRACE_METHOD_LINKAGE to show the target MethodHandle
• 182c215888fa2f58f9d1f4cfb32f1f45012b8d9f: 8295994: Remove left over InetAddressContainer class
• 78763fc8e0d6940f1c85ff8705733b8e6ae8e945: 8295000: java/util/Formatter/Basic test cleanup
• 907d583376dfab269ea25a6c036e390f3484065e: 8295323: Unnecessary HashTable usage in StyleSheet
• 2157145766f9789ade0940e9ae1715a3b74d508b: 8293858: Change PKCS7 code to use default SecureRandom impl instead of SHA1PRNG
• b8ad6cd98a7e4b577b888dc5f9d93c2e4d3bf177: 8294461: wrong effectively final determination by javac
• d6678952a6de4e5435dab65e7029021832454857: 8294399: (ch) Refactor some methods out of sun.nio.ch.UnixFileDispatcherImpl
• 628820f47ef9c9ad3cc62e68db9c4dbc7e659154: 8283093: JMX connections should default to using an ObjectInputFilter
• ... and 1134 more: https://git.openjdk.org/jdk/compare/0ca74f538e1a8a351cc0631c5fe397a74653ce6f...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@chhagedorn @SuperCoder7979 Pushed as commit cf5546b3ac63e305c0b9d040353503fb33d6ad7a.

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