RFR: 8323429: Missing C2 optimization for FP min/max when both inputs are same

[Christian Hagedorn]

On Thu, 11 Apr 2024 12:10:38 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

> Added C2 identity optimization for min/max calls, whereby if both inputs are the same, either is returned.
> 
> It includes an IR test to verify that the optimization gets applied. The optimization applies not only to floating points, but also long and ints. The test includes tests for all of those.
> 
> `BasicDoubleOpTest.vectorMax_8322090` has also been adjusted to match expectations after implementing the optimization.
> 
> I've run hotspot compiler tests successfully on x86_64.

Otherwise, the fix looks good!

test/hotspot/jtreg/compiler/intrinsics/math/TestMinMaxOpt.java line 26:

> 24: /**
> 25:  * @test
> 26:  * @bug 8287087

Suggestion:

 * @bug 8323429

test/hotspot/jtreg/compiler/intrinsics/math/TestMinMaxOpt.java line 27:

> 25:  * @test
> 26:  * @bug 8287087
> 27:  * @summary ...

You should add a summary.

test/hotspot/jtreg/compiler/intrinsics/math/TestMinMaxOpt.java line 29:

> 27:  * @summary ...
> 28:  * @library /test/lib /
> 29:  * @requires vm.compiler2.enabled

I don't think this `@requires` is required - we can also run the test without C2.

test/hotspot/jtreg/compiler/intrinsics/math/TestMinMaxOpt.java line 52:

> 50:     private static int testIntMin(int v) {
> 51:         return Math.min(v, v);
> 52:     }

You could also add `@Check` methods to sanity check that we actually get 42 back. This could be done like this (you can also have a look at [other examples](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/CheckedTestExample.java)):


@Check(test = "testIntMin")
public static void checkTest2(int result) {
    if (result != 42) {
        throw new RuntimeException("Incorrect result: " + result);
    }
}

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 822:

> 820:     public static final String MAX = PREFIX + "MAX" + POSTFIX;
> 821:     static {
> 822:         beforeMatchingNameRegex(MAX, "Max(I|L)");

For completeness, we should probably also add `F` and `D` here. I've quickly checked the usages uf `MAX` and we only seem to be using it in `failOn` conditions. So, this should be safe to do.

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

PR Review: https://git.openjdk.org/jdk/pull/18738#pullrequestreview-1994139787
PR Review Comment: https://git.openjdk.org/jdk/pull/18738#discussion_r1560995816
PR Review Comment: https://git.openjdk.org/jdk/pull/18738#discussion_r1560995511
PR Review Comment: https://git.openjdk.org/jdk/pull/18738#discussion_r1560996229
PR Review Comment: https://git.openjdk.org/jdk/pull/18738#discussion_r1561002832
PR Review Comment: https://git.openjdk.org/jdk/pull/18738#discussion_r1560993923