8341781: Improve Min/Max node identities #21439

jaskarth · 2024-10-10T02:59:14Z

Hi all,
This patch implements some missing identities for Min/Max nodes. It adds static type-based operand choosing for MinI/MaxI, such as the ones that MinL/MaxL use. In addition, it adds simplification for patterns such as Max(A, Max(A, B)) to Max(A, B) and Max(A, Min(A, B)) to A. These simplifications stem from the lattice identity rules. The main place I've seen this pattern is with MinL/MaxL nodes created during loop optimizations. Some examples of where this occurs include BigInteger addition/subtraction, and regex code. I've run some of the existing benchmarks and found some nice improvements:

                                                                Baseline                    Patch
Benchmark                                 Mode  Cnt       Score       Error  Units    Score       Error  Units  Improvement
BigIntegers.testAdd                       avgt   15      25.096 ±     3.936  ns/op   19.214  ±    0.521  ns/op  (+ 26.5%)
PatternBench.charPatternCompile           avgt    8     453.727 ±   117.265  ns/op   370.054 ±   26.106  ns/op  (+ 20.3%)
PatternBench.charPatternMatch             avgt    8     917.604 ±   121.766  ns/op   810.560 ±   38.437  ns/op  (+ 12.3%)
PatternBench.charPatternMatchWithCompile  avgt    8    1477.703 ±   255.783  ns/op  1224.460 ±   28.220  ns/op  (+ 18.7%)
PatternBench.longStringGraphemeMatches    avgt    8     860.909 ±   124.661  ns/op   743.729 ±   22.877  ns/op  (+ 14.6%)
PatternBench.splitFlags                   avgt    8     420.506 ±    76.252  ns/op   321.911 ±   11.661  ns/op  (+ 26.6%)

I've added some IR tests, and tier 1 testing passes on my linux machine. Reviews would be appreciated!

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-8341781: Improve Min/Max node identities (Enhancement - P4)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21439

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-10T02:59:43Z

👋 Welcome back jkarthikeyan! 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 · 2024-10-10T03:00:19Z

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

8341781: Improve Min/Max node identities

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

5cc150c: 8342979: Start of release updates for JDK 25
85fedbf: 8344607: Link Time Optimization - basic support for clang
5a0899f: 8345302: Building microbenchmarks require larger Java heap
1ece4f9: 8345514: Should use internal class name when calling ClassLoader.getResourceAsByteArray
ef8da28: 8345591: [aarch64] macroAssembler_aarch64.cpp compile fails ceil_log2 not declared
7513b13: 8328944: NMT reports "unknown" memory
691e692: 8345565: Remove remaining SecurityManager motivated APIs from sun.reflect.util
97b8a09: 8345339: JFR: Missing javadoc for RecordingStream::onMetadata
456c71d: 8343699: [aarch64] Bug in MacroAssembler::klass_decode_mode()
308357c: 8345578: New test in JDK-8343622 fails with a promoted build
... and 723 more: https://git.openjdk.org/jdk/compare/b6f745df5795341dab1fc049a188a9e70d563a1a...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.

openjdk · 2024-10-10T03:01:02Z

@jaskarth 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 · 2024-10-10T03:04:46Z

Webrevs

src/hotspot/share/opto/addnode.hpp

chhagedorn

Few comments, otherwise, looks good to me.

src/hotspot/share/opto/addnode.cpp

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdentities.java

test/hotspot/jtreg/compiler/vectorization/runner/BasicShortOpTest.java

chhagedorn · 2024-10-10T09:02:41Z

Your new test fails on Linux with -XX:UseAVX=0:

One or more @IR rules failed:

Failed IR Rules (8) of Methods (8)
----------------------------------
1) Method "public double compiler.c2.irTests.TestMinMaxIdentities.doubleMaxMax(double,double)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MAX_D#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MaxD.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

2) Method "public double compiler.c2.irTests.TestMinMaxIdentities.doubleMaxMin(double,double)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MIN_D#_", "1", "_#MAX_D#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MinD.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!
         * Constraint 2: "(\\d+(\\s){2}(MaxD.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

3) Method "public double compiler.c2.irTests.TestMinMaxIdentities.doubleMinMax(double,double)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MIN_D#_", "1", "_#MAX_D#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MinD.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!
         * Constraint 2: "(\\d+(\\s){2}(MaxD.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

4) Method "public double compiler.c2.irTests.TestMinMaxIdentities.doubleMinMin(double,double)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MIN_D#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MinD.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

5) Method "public float compiler.c2.irTests.TestMinMaxIdentities.floatMaxMax(float,float)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MAX_F#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MaxF.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

6) Method "public float compiler.c2.irTests.TestMinMaxIdentities.floatMaxMin(float,float)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MIN_F#_", "1", "_#MAX_F#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MinF.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!
         * Constraint 2: "(\\d+(\\s){2}(MaxF.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

7) Method "public float compiler.c2.irTests.TestMinMaxIdentities.floatMinMax(float,float)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MIN_F#_", "1", "_#MAX_F#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MinF.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!
         * Constraint 2: "(\\d+(\\s){2}(MaxF.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

8) Method "public float compiler.c2.irTests.TestMinMaxIdentities.floatMinMin(float,float)" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#MIN_F#_", "1"}, applyIfPlatform={}, applyIfPlatformOr={}, failOn={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
     > Phase "PrintIdeal":
       - counts: Graph contains wrong number of nodes:
         * Constraint 1: "(\\d+(\\s){2}(MinF.*)+(\\s){2}===.*)"
           - Failed comparison: [found] 0 = 1 [given]
           - No nodes matched!

Looks like we do not emit Min/MaxF/D nodes with UseAVX=0. I quickly checked the code and indeed, the intrinsics are only enabled if UseAVX >= 1:

jdk/src/hotspot/cpu/x86/x86.ad

Lines 1542 to 1549 in 1604255

    
           case Op_MaxD: 
        
           case Op_MaxF: 
        
           case Op_MinD: 
        
           case Op_MinF: 
        
             if (UseAVX < 1) { // enabled for AVX only 
        
               return false; 
        
             } 
        
             break;

You can probably just update your tests to exclude IR matching for this setup. Maybe you also want to double check the other architectures.

jaskarth · 2024-10-11T15:12:34Z

Thanks for the suggestions and testing, @liach and @chhagedorn! I've taken a look at the backend implementations, and it seems that aarch64 and RISC-V unconditionally support floating point Min/Max while x64 only supports them with UseAVX >= 1, as described. I made it so that the test only runs when it matches that criteria. I've pushed a commit that should address all the suggestions here.

chhagedorn

Looks good, thanks for the update! I'll give this another spinning in our testing.

chhagedorn · 2024-10-17T05:25:36Z

Testing passed!

eme64

Drive-by comments. Generally looks reasonable, nice work.
I have 2 comments below.

eme64 · 2024-10-30T08:50:33Z

src/hotspot/share/opto/addnode.cpp


+Node* MaxINode::Identity(PhaseGVN* phase) {
+  const TypeInt* t1 = phase->type(in(1))->is_int();
+  const TypeInt* t2 = phase->type(in(2))->is_int();


Could any input be TOP?

I think we can't encounter TOP here because we run Value() before Identity(), so if TOP is returned by Value() the idealization process exits to return the top node before running Identity().

eme64 · 2024-10-30T08:54:33Z

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdentities.java

+ * @bug 8341781
+ * @summary Test identities of MinNodes and MaxNodes.
+ * @key randomness
+ * @requires (os.simpleArch == "x64" & vm.cpu.features ~= ".*avx.*") | os.arch == "aarch64" | os.arch == "riscv64"


Is there a chance we can add these requires to the @IR rules instead? That way we can still do the result verification on all other platforms, which could be valuable on its own.

From my understanding this isn't possible as-is since CPU features seem to be checked regardless of whether the architecture supports it or not, so we can't simply check for AVX because that would fail on aarch64 and riscv64. I think we could work around this with applyIfCPUFeatureOr = {"avx", "true", "asimd", "true", "rvv", "true"} to force a check for all 3 platforms but it'd be filtering more platforms than strictly necessary.

Which platforms would be filtered "more platforms than strictly necessary"?

With the workaround to check for CPU features on all 3 platforms, we'd be not checking the IR when asimd = false or rvv = false, but the IR check should pass with those features too.

I think your tags are actually available, with applyIfPlatform. Look at irTestingPlatforms in https://github.com/openjdk/jdk/blame/master/test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java#L69

The usage of these is quite rare - usually we focus more on the CPU features, and not the platform tags ;)

Ah, I was meaning that with applyIfPlatformOr = {"x64", "true", "aarch64", "true", "riscv64", "true"} we would still need at least applyIfCPUFeature = {"avx", true"} because on x86 we only make Min/MaxF and Min/MaxD with AVX. But that applyIfCPUFeature will check for AVX on aarch64 and riscv64 as well, but will fail because it's not available for those platforms. That's why I suggested the workaround of checking the other CPU features, to make the test at least run on the other platforms. It'd be nice to be able to express platform and CPU feature combinations like with @requires, but the use-case here is pretty niche.

First: yes, I'm fine with just using CPU features - it will IR test it on fewer platforms than maybe desired, but that is ok, I think.

I guess the issue with @IR is that it only allows AND or OR clauses ... and not the more complicated mix of & and | from @requires. But in theory, you can just have multiple @IR rules (that simulates the OR):

One for x64 (applyIfPlatform) and avx (applyIfCPUFeature)

One IR rule for each of: aarch64 and riscv64 (applyIfPlatform)

But again, I'm ok with only checking for CPU features... it is more simple.

@chhagedorn you may be interested in this conversation as well ;)

I hadn't considered using multiple IR checks, that would definitely work as well. I've pushed a commit that just uses the CPU features, to keep it simple. Let me know what you think!

eme64 · 2024-11-01T07:08:31Z

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdentities.java

+    // As JDK-8307513 adds intrinsics for the methods, the Long tests are disabled until then.
+
+    @Test
+//     @IR(applyIfPlatform = { "riscv64", "false" }, phase = { CompilePhase.BEFORE_MACRO_EXPANSION }, counts = { IRNode.MIN_L, "1" })


I would say you should make them negative for now, i.e. make them failOn. Otherwise we won't catch these cases when JDK-8307513 gets integrated ;)

Sounds good, I've pushed a commit that makes the tests pass now but fail when 8307513 is integrated.

eme64 · 2024-11-01T07:10:40Z

The IR rules look ok to me. Nice progress :)

chhagedorn

The IR rule updates look good. Maybe you want to wait with integrating this until after the fork next Thursday. So, this only goes into JDK 25.

jaskarth · 2024-12-04T16:38:45Z

Thanks for the re-review! I think that's a good idea, I'll integrate it after JDK 24 is forked.

jaskarth · 2024-12-10T22:31:25Z

Since it's after the fork, I'll integrate it now. Thanks again for the reviews everyone!
/integrate

openjdk · 2024-12-10T22:32:23Z

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

4c39e9f: 8344924: Default CA certificates loaded despite request to use custom keystore
0f03554: 8342469: Improve API documentation for java.lang.classfile.instruction
9bd70ec: 8345888: Broken links in the JDK 24 JavaDoc API documentation, build 27
1def2d8: 8345700: tier{1,2,3}_compiler don't cover all compiler tests
d6b5264: 8345746: Remove :resourcehogs/compiler from :hotspot_slow_compiler
4f855d1: 8345818: Fix SM cleanup of parsing of System property resource.bundle.debug
a24b08f: 8345669: RISC-V: fix client build failure due to AlignVector after JDK-8343827
25d9deb: 8335367: [s390] Add support for load immediate on condition instructions.
a6277bb: 8340212: -Xshare:off -XX:CompressedClassSpaceBaseAddress=0x40001000000 crashes on macos-aarch64
165fd84: 8342987: Update --release 24 symbol information for JDK 24 build 27
... and 779 more: https://git.openjdk.org/jdk/compare/b6f745df5795341dab1fc049a188a9e70d563a1a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-10T22:32:34Z

@jaskarth Pushed as commit 29d648c.

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

Min/Max identities

af771cf

openjdk bot added the rfr Pull request is ready for review label Oct 10, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 10, 2024

liach reviewed Oct 10, 2024

View reviewed changes

src/hotspot/share/opto/addnode.hpp Show resolved Hide resolved

chhagedorn reviewed Oct 10, 2024

View reviewed changes

Suggestions from review

b4b9614

chhagedorn approved these changes Oct 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 15, 2024

eme64 reviewed Oct 30, 2024

View reviewed changes

jaskarth added 2 commits October 30, 2024 22:59

Merge branch 'master' into minmax_identities

dd4f9ec

Add platform checks to IR

39f7d04

openjdk bot removed the ready Pull request is ready to be integrated label Oct 31, 2024

eme64 reviewed Nov 1, 2024

View reviewed changes

Make long tests check IR

fc484f6

chhagedorn approved these changes Nov 27, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 27, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 10, 2024

openjdk bot closed this Dec 10, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Dec 10, 2024

openjdk bot removed the rfr Pull request is ready for review label Dec 10, 2024

8341781: Improve Min/Max node identities #21439

8341781: Improve Min/Max node identities #21439

Uh oh!

Conversation

jaskarth commented Oct 10, 2024 • 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 Oct 10, 2024

Uh oh!

openjdk bot commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 10, 2024

Uh oh!

mlbridge bot commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chhagedorn commented Oct 10, 2024

Uh oh!

jaskarth commented Oct 11, 2024

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn commented Oct 17, 2024

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 commented Nov 1, 2024

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

jaskarth commented Dec 4, 2024

Uh oh!

jaskarth commented Dec 10, 2024

jaskarth commented Oct 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 10, 2024 •

edited

Loading

mlbridge bot commented Oct 10, 2024 •

edited

Loading