RFR: 8350835: C2 SuperWord: assert/wrong result when using Float.float16ToFloat with byte instead of short input

[Emanuel Peter]

On Fri, 7 Mar 2025 01:56:49 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

> Float.float16ToFloat generates wrong vectorized code in product build and asserts in fastdebug/debug when argument is of type byte, int, or long array. The short term solution is to not auto vectorize in these cases.
> 
> Review comments are welcome.
> 
> Best Regards,
> Sandhya

Changes requested by epeter (Reviewer).

test/hotspot/jtreg/compiler/vectorization/TestFloat16ToFloatConv.java line 29:

> 27: * @bug 8350835
> 28: * @summary Test bug fix for JDK-8350835 discovered through Template Framework
> 29: * @requires vm.compiler2.enabled

Suggestion:


Is this restriction necessary? I generally prefer running tests on all platforms, and only restricting IR rules. That way we can get more test coverage with result verification.

test/hotspot/jtreg/compiler/vectorization/TestFloat16ToFloatConv.java line 31:

> 29: * @requires vm.compiler2.enabled
> 30: * @library /test/lib /
> 31: * @run main/othervm -XX:-TieredCompilation -XX:CompileOnly=compiler.vectorization.TestFloat16ToFloatConv::test* compiler.vectorization.TestFloat16ToFloatConv

Are the additional flags really necessary for reproducing the bug? I would suspect not really. The IR framework already takes care of ensuring we run with C2 compilation.

test/hotspot/jtreg/compiler/vectorization/TestFloat16ToFloatConv.java line 47:

> 45:     private static int[] aI = new int[SIZE];
> 46:     private static long[] aL = new long[SIZE];
> 47:     private static float[] goldB, goldS, goldI, goldL;

Are you testing for `char` as well?

test/hotspot/jtreg/compiler/vectorization/TestFloat16ToFloatConv.java line 55:

> 53:             aI[i] = RANDOM.nextInt();
> 54:             aL[i] = RANDOM.nextLong();
> 55:         }

I would prefer if we could start using `Generators`. There is a `fill` method for arrays. It generates more "interesting" values. It is not super relevant here, but it would be nice if we made this common practice now ;)

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

PR Review: https://git.openjdk.org/jdk/pull/23939#pullrequestreview-2670202061
PR Review Comment: https://git.openjdk.org/jdk/pull/23939#discussion_r1986858150
PR Review Comment: https://git.openjdk.org/jdk/pull/23939#discussion_r1986860021
PR Review Comment: https://git.openjdk.org/jdk/pull/23939#discussion_r1986861107
PR Review Comment: https://git.openjdk.org/jdk/pull/23939#discussion_r1986863298