RFR: 8341781: Improve Min/Max node identities [v2]

[Emanuel Peter]

On Fri, 11 Oct 2024 15:15:54 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!
>
> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Suggestions from review

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

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

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

Could any input be `TOP`?

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

> 33:  * @summary Test identities of MinNodes and MaxNodes.
> 34:  * @key randomness
> 35:  * @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.

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

PR Review: https://git.openjdk.org/jdk/pull/21439#pullrequestreview-2404033717
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1822143491
PR Review Comment: https://git.openjdk.org/jdk/pull/21439#discussion_r1822149392