RFR: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long)

[Galder Zamarreño]

On Thu, 7 Nov 2024 10:18:14 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

>> Overall, looks fine.
>> 
>> So, there will be `inline_min_max`, `inline_fp_min_max`, and `inline_long_min_max` which slightly vary. I'd prefer to see them unified. (Or, at least, enhance `inline_min_max` to cover `minL`/maxL` cases).
>> 
>> Also, it's a bit confusing to see int variants names w/o basic type (`_min`/`_minL` vs `_minI`/`_minL`).  Please, clean it up along the way. (FTR I'm also fine handling the renaming as a separate change.)
>
>> Overall, looks fine.
>> 
>> So, there will be `inline_min_max`, `inline_fp_min_max`, and `inline_long_min_max` which slightly vary. I'd prefer to see them unified. (Or, at least, enhance `inline_min_max` to cover `minL`/maxL` cases).
>> 
>> Also, it's a bit confusing to see int variants names w/o basic type (`_min`/`_minL` vs `_minI`/`_minL`). Please, clean it up along the way. (FTR I'm also fine handling the renaming as a separate change.)
> 
> @iwanowww I applied the changes you suggested. Could you review them?

> @galderz Thanks for taking this task on! Had a quick look at it. So auto-vectorization in SuperWord should now be working, right? If yes:
> 
> It would be nice if you tested both for `IRNode.MIN_VL` and `IRNode.MIN_REDUCTION_V`, the same for max.
> 
> You may want to look at these existing tests, to see what other tests there are for the `int` version: `test/hotspot/jtreg/compiler/loopopts/superword/MinMaxRed_Int.java` `test/hotspot/jtreg/compiler/c2/irTests/TestIfMinMax.java` `test/hotspot/jtreg/compiler/vectorization/TestAutoVecIntMinMax.java` `test/hotspot/jtreg/compiler/c2/TestMinMaxSubword.java` There may be some duplicates already here... not sure. 

+1 to adding such tests. I'm looking into it right now. It's a bit confusing how the tests are spread around (and duplication?) but I'm currently leaning towards adding a `compiler/loopopts/superword/MinMaxRed_Long.java`.

> And maybe you need to check what to do about probabilities as well.

I will add probabilities logic (50, 80, 100) to control data, but you can already see that from https://github.com/openjdk/jdk/pull/20098#issuecomment-2379386872 that with the patch in an AVX512 system min/max reduction nodes will appear in all probabilities.

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

PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2530905728