RFR: 8341781: Improve Min/Max node identities

[Christian Hagedorn]

On Thu, 10 Oct 2024 02:59:14 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

> 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](https://en.wikipedia.org/wiki/Lattice_(order)#As_algebraic_structure). 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!

Few comments, otherwise, looks good to me.

src/hotspot/share/opto/addnode.cpp line 1478:

> 1476:       }
> 1477: 
> 1478:       // If the operations are different return the operand, as Max(A, Min(A, B)) == A if the value isn't a floating point value,

Suggestion:

      // If the operations are different return the operand 'A', as Max(A, Min(A, B)) == A if the value isn't a floating point value,

src/hotspot/share/opto/addnode.cpp line 1479:

> 1477: 
> 1478:       // If the operations are different return the operand, as Max(A, Min(A, B)) == A if the value isn't a floating point value,
> 1479:       // as if B == NaN the identity doesn't hold.

Reads as "as if". Maybe rephrase to
Suggestion:

      // For floating points, the identity does not hold if B == NaN.

?

src/hotspot/share/opto/addnode.cpp line 1485:

> 1483:       }
> 1484: 
> 1485:       return nullptr;

I guess you can remove this since we return nullptr below anyway.
Suggestion:

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdentities.java line 116:

> 114: 
> 115:     @Test
> 116:     @IR(applyIfPlatform = { "riscv64", "false" }, phase = { CompilePhase.BEFORE_MACRO_EXPANSION }, counts = { IRNode.MIN_L, "1" })

Can you add a comment here why we cannot apply the rules for riscv?

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdentities.java line 122:

> 120: 
> 121:     @Test
> 122:     @IR(applyIfPlatform = { "riscv64", "false" }, failOn = { IRNode.MIN_L, IRNode.MAX_L })

Since `MinL/MaxL` are expanded in macro expansion, this rule will also succeed even if the optimization is not applied. I suggest to also add `phase = CompilePhase.BEFORE_MACRO_EXPANSION`. Same below.

test/hotspot/jtreg/compiler/vectorization/runner/BasicShortOpTest.java line 220:

> 218:         short[] res = new short[SIZE];
> 219:         for (int i = 0; i < SIZE; i++) {
> 220:             res[i] = (short) Math.min(a[i], b[i]);

I guess without this change, this collapses to a constant which enables vectorization which was not expected before?

-------------

PR Review: https://git.openjdk.org/jdk/pull/21439#pullrequestreview-2359045864
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1794700959
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1794705393
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1794707261
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1794711053
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1794714816
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1794720021