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

[Emanuel Peter]

On Fri, 26 Jan 2024 16:05:45 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:
> 
>   Don't transform highly predictable branches

Changes requested by epeter (Reviewer).

src/hotspot/share/opto/cfgnode.cpp line 1788:

> 1786:   IfNode* iff = region->in(1)->in(0)->as_If();
> 1787:   BoolNode* bol = iff->in(1)->as_Bool();
> 1788:   Node* cmp = bol->in(1);

Could some of these values be const, or even TOP?

src/hotspot/share/opto/cfgnode.cpp line 1797:

> 1795: 
> 1796:   int false_path = 3 - true_path;
> 1797:   bool is_min = false;

`is_min` is a bit far removed from when it is actually used.

src/hotspot/share/opto/cfgnode.cpp line 1802:

> 1800: 
> 1801:   // Ensure comparison is an integral type
> 1802:   if (opcode == Op_CmpI || opcode == Op_CmpL) {

Nicer would be "bailout" ifs:
Suggestion:

  if (opcode != Op_CmpI && opcode != Op_CmpL) { return nullptr; }
  bool is_long = opcode == Op_CmpL;

  // Only accept canonicalized le and lt comparisons
  int test = bol->_test._test;
  if (test != BoolTest::le && test != BoolTest::lt) { return nullptr; }


It is nice to have all the "bailout" checks as early as possible, and then do the "meat" of the optimization afterward.

src/hotspot/share/opto/cfgnode.cpp line 1824:

> 1822:   } else {
> 1823:     return nullptr;
> 1824:   }

Maybe also have variables `phi_true/false`, would make the code easier to read.

src/hotspot/share/opto/cfgnode.cpp line 1843:

> 1841:       return new MaxINode(cmp_true, cmp_false);
> 1842:     }
> 1843:   }

Please use `build_min_max_int`. And maybe create a method for `longs` as well.

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

PR Review: https://git.openjdk.org/jdk/pull/17574#pullrequestreview-1867332408
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481183143
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481197958
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481188772
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481206169
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481178505