8312547: Max/Min nodes Value implementation could be improved

[merykitty]

Hi,

This patch removes the early return in AddNode::Value in case one of the inputs is a bottom, which may affect the value calculation of nodes such as Min/MaxNode.

Please kindly review, thanks very much.

---

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-8312547: Max/Min nodes Value implementation could be improved (Enhancement - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to ef2d3dfb
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15021

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@merykitty 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]

Webrevs

• 03: Full - Incremental (0a117343)
• 02: Full - Incremental (4ae1ad36)
• 01: Full - Incremental (ef2d3dfb)
• 00: Full (312088f7)

[merykitty]

I don't think Type::BOTTOM can appear here, there are other nodes (such as convert nodes) that do not check for Type::BOTTOM. Maybe someone could shed light on this, please?

[VladimirKempik]

could you please check your change with jmh test ( numbers before and after the patch) : org/openjdk/bench/vm/compiler/MaxMinOptimizeTest.java

just want to make sure it doesn't regress

[merykitty]

@VladimirKempik Thanks for taking a look, the results look the same both before and after the patch, which is expected given none of them would benefit from more precise values of max/min nodes, these are the numbers:

                                        Before           After
Benchmark                Mode  Cnt   Score   Error   Score   Error  Units
MaxMinOptimizeTest.dAdd  avgt   12  59.865 ± 0.445  59.822 ± 0.323  us/op
MaxMinOptimizeTest.dMax  avgt   12  61.109 ± 3.639  59.703 ± 0.187  us/op
MaxMinOptimizeTest.dMin  avgt   12  59.961 ± 0.389  59.788 ± 0.281  us/op
MaxMinOptimizeTest.dMul  avgt   12  59.477 ± 0.277  59.446 ± 0.337  us/op
MaxMinOptimizeTest.fAdd  avgt   12  59.230 ± 0.315  59.103 ± 0.174  us/op
MaxMinOptimizeTest.fMax  avgt   12  59.344 ± 0.422  60.171 ± 1.672  us/op
MaxMinOptimizeTest.fMin  avgt   12  59.581 ± 0.606  59.187 ± 0.487  us/op
MaxMinOptimizeTest.fMul  avgt   12  59.001 ± 0.236  58.948 ± 0.310  us/op

[VladimirKempik]

@VladimirKempik Thanks for taking a look, the results look the same both before and after the patch, which is expected given none of them would benefit from more precise values of max/min nodes, these are the numbers:

                                        Before           After
Benchmark                Mode  Cnt   Score   Error   Score   Error  Units
MaxMinOptimizeTest.dAdd  avgt   12  59.865 ± 0.445  59.822 ± 0.323  us/op
MaxMinOptimizeTest.dMax  avgt   12  61.109 ± 3.639  59.703 ± 0.187  us/op
MaxMinOptimizeTest.dMin  avgt   12  59.961 ± 0.389  59.788 ± 0.281  us/op
MaxMinOptimizeTest.dMul  avgt   12  59.477 ± 0.277  59.446 ± 0.337  us/op
MaxMinOptimizeTest.fAdd  avgt   12  59.230 ± 0.315  59.103 ± 0.174  us/op
MaxMinOptimizeTest.fMax  avgt   12  59.344 ± 0.422  60.171 ± 1.672  us/op
MaxMinOptimizeTest.fMin  avgt   12  59.581 ± 0.606  59.187 ± 0.487  us/op
MaxMinOptimizeTest.fMul  avgt   12  59.001 ± 0.236  58.948 ± 0.310  us/op

Thank you

[TobiHartmann]

Looks good to me.

[openjdk]

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

8312547: Max/Min nodes Value implementation could be improved

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

• 2624324: 8219567: Name of first parameter of RandomAccessFile(String,String) is inconsistent
• 8e4240c: 8314850: SharedRuntime::handle_wrong_method() gets called too often when resolving Continuation.enter
• d9fc6b6: 8314759: VirtualThread.parkNanos timeout adjustment when pinned should be replaced
• acaab6f: 8306040: HttpResponseInputStream.available() returns 1 on empty stream
• 76b9011: 8314656: GHA: No need for Debian ports keyring installation after JDK-8313701
• e36620d: 8314554: Debian/Ubuntu should not link OpenJDK with --as-needed link option
• 97b94cb: 8314883: Java_java_util_prefs_FileSystemPreferences_lockFile0 write result errno in missing case
• 75e19e0: 8314819: [JVMCI] HotSpotJVMCIRuntime.lookupType throws unexpected ClassNotFoundException
• c418933: 8313530: VM build without C2 fails after JDK-8312579
• de0e46c: 8303815: Improve Metaspace test speed
• ... and 46 more: https://git.openjdk.org/jdk/compare/ed0f75f2666f61ec3d8ccb78594a21db9f1be50a...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.

[merykitty]

@TobiHartmann Thanks for your reviews, I have adjusted the code style there.

[merykitty]

Hi, may I have a second review, please?

[vnkozlov]

I am fine with fix and cleaning you did.

[merykitty]

@vnkozlov Thanks a lot for your reviews, I will integrate the change.

[merykitty]

/integrate

[openjdk]

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

• 7342f5a: 8314333: Update com/sun/jdi/ProcessAttachTest.java to use ProcessTools.createTestJvm(..)
• f139f30: 8315033: Problemlist java/lang/template/StringTemplateTest.java
• f2383b3: 8314063: The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection
• dcd6e75: 8314990: Generational ZGC: Strong OopStorage stats reported as weak roots
• 002b594: 8312749: Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))
• d024059: 8314951: VM build without C2 still fails after JDK-8313530
• 2624324: 8219567: Name of first parameter of RandomAccessFile(String,String) is inconsistent
• 8e4240c: 8314850: SharedRuntime::handle_wrong_method() gets called too often when resolving Continuation.enter
• d9fc6b6: 8314759: VirtualThread.parkNanos timeout adjustment when pinned should be replaced
• acaab6f: 8306040: HttpResponseInputStream.available() returns 1 on empty stream
• ... and 52 more: https://git.openjdk.org/jdk/compare/ed0f75f2666f61ec3d8ccb78594a21db9f1be50a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@merykitty Pushed as commit 837cf85.

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