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

[Emanuel Peter]

On Tue, 10 Dec 2024 09:10:04 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.)
>> 
>> @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.

@galderz Yes, there is significant duplication, sadly. Often there were old tests there, but then one comes along and sees that one wants to have more comprehensive tests. So one adds it, but does not feel 100% comfortable removing old tests. A little bit of duplication is probably ok. Often, there are still subtle differences, and sometimes those end up mattering.

`compiler/loopopts/superword/MinMaxRed_Long.java` sounds like a good idea.

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

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