On Tue, 14 Jan 2025 05:07:55 GMT, Galder Zamarreño <gal...@openjdk.org> wrote:
>> @galderz So you want me to review again? > > @eme64 I've fixed the test issue, it's ready to be reviewed > @galderz I don't remember from above, but did you ever run the Long Min/Max > benchmarks from this? https://github.com/openjdk/jdk/pull/21032 Would just be nice to see that they have an improvement after this change :) Looking at the benchmark the arrays are loaded with random data with no control over which branch side will be taken. So there's no guarantees that you will see an improvement for the reasons I explained in https://github.com/openjdk/jdk/pull/20098#issuecomment-2379386872. To summarise what was observed there: * In AVX-512 you will only see an improvement when one of the min/max branches is taken ~100% of the time. * In non-AVX-512 this patch will create a regression when one of the min/max branches is taken ~100% of time. If it helps I'm happy to document this in detail in the `MinMaxVector` benchmark added here. I would expect a similar thing to happen when it comes to asimd envs with max vector size >= 32 (e.g. Graviton 3). Those will see vectorization occur and improvements kick in at 100%. Other systems (e.g. Graviton 4) will see a regression at 100%. This means that your work in https://github.com/openjdk/jdk/pull/20098#discussion_r1901576209 to avoid the max vector size limitation might become more important once my PR here goes in. I'm wondering if the long min/max benchmarks introduced in https://github.com/openjdk/jdk/pull/21032 should remain because their results are not predictable and that's not a good situation. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20098#issuecomment-2598645987
