On Thu, 12 Dec 2024 12:03:11 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>>> @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. @eme64 I've addressed all your comments except aarch64 testing. `asimd` is not enough, you need `sve` for this, but I'm yet to make it work even with `sve`, something's up and need to debug it further. @jaskarth FYI I've adjusted the expectations in `TestMinMaxIdentities` after this change (thx for adding the test!). Check if there's any comments/changes you'd like. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2549259008
