RFR: 8324655: Identify integer minimum and maximum patterns created with if statements [v6]

[Emanuel Peter]

On Wed, 6 Mar 2024 06:13:02 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

>> Hi all, I've created this patch which aims to convert common integer mininum and maximum patterns created using if statements into Min and Max nodes. These patterns are usually in the form of `a > b ? a : b` and similar, as well as patterns such as `if (a > b) b = a;`. While this transform doesn't generally improve code generation it's own, it simplifies control flow and creates new opportunities for vectorization.
>> 
>> I've created a benchmark for the PR, and I've attached some data from my (Zen 3) machine:
>> 
>>                                                 Baseline                     Patch            Improvement
>> Benchmark                   Mode   Cnt  Score      Error  Units    Score       Error  Units
>> IfMinMax.testReductionInt   avgt   15  500.307 ±  16.687  ns/op    509.383 ±  32.645  ns/op   (no change)*
>> IfMinMax.testReductionLong  avgt   15  493.184 ±  17.596  ns/op    513.587 ±  28.339  ns/op   (no change)*
>> IfMinMax.testSingleInt      avgt   15    3.588 ±   0.540  ns/op      2.965 ±   1.380  ns/op   (no change)
>> IfMinMax.testSingleLong     avgt   15    3.673 ±   0.128  ns/op      3.506 ±   0.590  ns/op   (no change)
>> IfMinMax.testVectorInt      avgt   15  340.425 ±  13.123  ns/op     59.689 ±   7.509  ns/op   + 5.7x
>> IfMinMax.testVectorLong     avgt   15  326.420 ±  15.554  ns/op    117.190 ±   5.622  ns/op   + 2.8x
>> 
>> 
>> * After writing this benchmark I discovered that the compiler doesn't seem to create some simple min/max reductions, even when using Math.min/max() directly. Is this known or should I create a followup RFE for this?
>> 
>> The patch passes tier 1-3 testing on linux x64. Reviews or comments would be appreciated!
>
> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Change transform to work on CMoves

Nice work, I think this looks much better now!
I'm currently a bit tight on time, I'll run the benchmark on my next pass ;)

src/hotspot/share/opto/movenode.cpp line 189:

> 187: 
> 188: // Try to identify min/max patterns in CMoves
> 189: static Node* is_minmax(PhaseGVN* phase, Node* cmov) {

I'm not a fan of `is_...` methods that do more than a check, but actually have a side-effect.
I also suggest that `cmov` should already have a `CMovNode` type, and there should be an assert here.

I would probably do it similar to `AddNode::IdealIL` and `AddPNode::Ideal_base_and_offset`: call it `CMoveNode::IdealIL_minmax`. But add an assert to check for int or long.

src/hotspot/share/opto/movenode.cpp line 322:

> 320:   if (phase->C->post_loop_opts_phase()) {
> 321:     return nullptr;
> 322:   }

Putting the condition here would prevent any future optimization further down not to be executed. I think you should rather put this into the `is_minmax` method. Maybe this condition is now only relevant for `long`, but I think it would not hurt to also have it also for `int`, right?

test/hotspot/jtreg/compiler/c2/irTests/TestIfMinMax.java line 139:

> 137:     public long testMaxL2E(long a, long b) {
> 138:         return a <= b ? b : a;
> 139:     }

I assume some of the `long` patterns should also have become MaxL/MinL in some phase, right? Is there maybe some phase where the IR would actually show that? You can target the IR rule to a phase, I think. Would be worth a try.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17574#pullrequestreview-1921796112
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1515754879
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1515765294
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1515768176