RFR: 8373396: Min and Max Ideal missing AddNode::Ideal optimisations

[Emanuel Peter]

On Thu, 11 Dec 2025 16:02:24 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

> `MaxI` and `MinI` are missing `AddNode::Ideal` optimizations. These optimizations include commutation, flattening, pushing constants...etc. The PR changes `MaxINode::Ideal` and `MinINode::Ideal` to call `AddNode::Ideal`. Long versions already call `AddNode::Ideal` so nothing to change there.
> 
> The PR also includes a template framework generated test (cc @eme64) that verifies that all of `AddNode::Ideal` optimizations now apply correctly for min/max for longs and ints. Long tests have been added to validate that both ints and longs produce the same results.
> 
> Fixing this issue indirectly fixes `compiler/codegen/TestBooleanVect.java` when run with `-XX:VerifyIterativeGVN=1110`, which was failing due to `min` not having one of those optimisations. However, this PR does not make changes to `PhaseIterGVN::verify_Identity_for` because there are additional failures observed with min/max for integers in JDK-8373134. Therefore, changes there will in the PR for JDK-8373134 instead.
> 
> If you look at `PhaseIterGVN::verify_Ideal_for`, it contains. This looks like it could be removed in this PR as it looks like they were quite likely disabled due to the issue here. However, it's unclear what test was failing here (@eme64 ?):
> 
> 
>     // MinINode::Ideal
>     // Did not investigate, but there are some patterns that might
>     // need more notification.
>     case Op_MinI:
>     case Op_MaxI: // preemptively removed it as well.
>       return false;
> 
> 
> I've run tier1-3 tests on linux/x64 and they passed.

@galderz Nice, this looks like a good use-case of the template framework, it reduces the test size!

It still feels a bit boiler-plate-y ... but it is a step in the right direction for sure ☺

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdeal.java line 30:

> 28:  * @modules java.base/jdk.internal.misc
> 29:  * @library /test/lib /
> 30:  * @run driver compiler.c2.irTests.TestMinMaxIdeal

Suggestion:

 * @run driver ${test.main.class}

Also: please don't put any new tests in `irTests`. Rather put it in a directory based on the topic.

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdeal.java line 56:

> 54:         String templatedPackage ="compiler.c2.templated";
> 55:         String templatedClassName ="MinMaxIdeal";
> 56:         String templatedFQN = "%s.%s".formatted(templatedPackage, templatedClassName);

That looks a bit convoluted. Why not just use the final string?

test/hotspot/jtreg/compiler/c2/irTests/TestMinMaxIdeal.java line 74:

> 72:         testTemplateTokens.add(new TestGenerator(Op.MAX_I).generate());
> 73:         testTemplateTokens.add(new TestGenerator(Op.MIN_L).generate());
> 74:         testTemplateTokens.add(new TestGenerator(Op.MAX_L).generate());

Why not use `Op.values()` -> `List`, then iterate over that?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28770#pullrequestreview-3571434582
PR Review Comment: https://git.openjdk.org/jdk/pull/28770#discussion_r2613876244
PR Review Comment: https://git.openjdk.org/jdk/pull/28770#discussion_r2613880453
PR Review Comment: https://git.openjdk.org/jdk/pull/28770#discussion_r2614178984