On Tue, 16 Dec 2025 03:31:07 GMT, Eric Fang <[email protected]> wrote:
>> The original implementation of UMIN/UMAX reductions in JDK-8346174 used
>> incorrect identity values in the Java implementation and test code.
>>
>> Problem:
>> --------
>> UMIN was using MAX_OR_INF (signed maximum value) as the identity:
>> - Byte.MAX_VALUE (127) instead of max unsigned byte (255)
>> - Short.MAX_VALUE (32767) instead of max unsigned short (65535)
>> - Integer.MAX_VALUE instead of max unsigned int (-1)
>> - Long.MAX_VALUE instead of max unsigned long (-1)
>>
>> UMAX was using MIN_OR_INF (signed minimum value) as the identity:
>> - Byte.MIN_VALUE (-128) instead of 0
>> - Short.MIN_VALUE (-32768) instead of 0
>> - Integer.MIN_VALUE instead of 0
>> - Long.MIN_VALUE instead of 0
>>
>> This caused incorrect result. For example:
>> UMAX([42,42,...,42]) returned 128 instead of 42
>>
>> Solution:
>> ---------
>> Use correct unsigned identity values:
>> - UMIN: ($type$)-1 (maximum unsigned value)
>> - UMAX: ($type$)0 (minimum unsigned value)
>>
>> Changes:
>> --------
>> - X-Vector.java.template: Fixed identity values in reductionOperations
>> - gen-template.sh: Fixed identity values for test code generation
>> - templates/Unit-header.template: Updated copyright year to 2025
>> - Regenerated all Vector classes and test files
>>
>> Testing:
>> --------
>> All types (byte/short/int/long) now return correct results in both
>> interpreter mode (-Xint) and compiled mode.
>
> Eric Fang has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains four commits:
>
> - Merge branch 'master' into JDK-8372978-fix-umin-umax-identity
> - Declare two constants for UMIN/UMAX reduction identity values
> - Merge branch 'master' into JDK-8372978-fix-umin-umax-identity
> - 8372978: [VectorAPI] Fix incorrect identity values in UMIN/UMAX reductions
>
> The original implementation of UMIN/UMAX reductions in JDK-8346174
> used incorrect identity values in the Java implementation and test code.
>
> Problem:
> --------
> UMIN was using MAX_OR_INF (signed maximum value) as the identity:
> - Byte.MAX_VALUE (127) instead of max unsigned byte (255)
> - Short.MAX_VALUE (32767) instead of max unsigned short (65535)
> - Integer.MAX_VALUE instead of max unsigned int (-1)
> - Long.MAX_VALUE instead of max unsigned long (-1)
>
> UMAX was using MIN_OR_INF (signed minimum value) as the identity:
> - Byte.MIN_VALUE (-128) instead of 0
> - Short.MIN_VALUE (-32768) instead of 0
> - Integer.MIN_VALUE instead of 0
> - Long.MIN_VALUE instead of 0
>
> This caused incorrect result. For example:
> UMAX([42,42,...,42]) returned 128 instead of 42
>
> Solution:
> ---------
> Use correct unsigned identity values:
> - UMIN: ($type$)-1 (maximum unsigned value)
> - UMAX: ($type$)0 (minimum unsigned value)
>
> Changes:
> --------
> - X-Vector.java.template: Fixed identity values in reductionOperations
> - gen-template.sh: Fixed identity values for test code generation
> - templates/Unit-header.template: Updated copyright year to 2025
> - Regenerated all Vector classes and test files
>
> Testing:
> --------
> All types (byte/short/int/long) now return correct results in both
> interpreter mode (-Xint) and compiled mode.
test/jdk/jdk/incubator/vector/templates/Kernel-Reduction-Masked-op-func.template
line 12:
> 10: $abstractvectortype$ av =
> $abstractvectortype$.fromArray(SPECIES, a, i);
> 11: r[i] = av.reduceLanes(VectorOperators.[[TEST]], vmask);
> 12: ra = [[TEST_OP]](ra, r[i]);
It's probably ok to merge the two cases, but to be conservative assign to a
local variable rather than reading from the array e.g.,
$type$ v = av.reduceLanes(VectorOperators.[[TEST]], vmask);
r[i] = v;
ra = [[TEST_OP]](ra, v);
test/jdk/jdk/incubator/vector/templates/Unit-Identity-test.template line 3:
> 1:
> 2: @Test(dataProvider = "$type$UnaryOpProvider")
> 3: static void testIdentityValues(IntFunction<$type$[]> fa) {
Since we already have the scalar operation we can generalize and reuse e.g., to
test the following: (id, x) == x, (x, id) == x, (id, id) == id. I believe we
could add the additional test to `Unit-Reduction-op.template` and
`Unit-Reduction-op-func.template`. WDYT?
test/jdk/jdk/incubator/vector/templates/Unit-Identity-test.template line 60:
> 58:
> 59: @Test(dataProvider = "$type$UnaryOpProvider")
> 60: static void testMaskedReductionIdentityAllFalse(IntFunction<$type$[]>
> fa) {
Do we need this test since we already have boolean data sets with all false
values that is used for the mask tests
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28692#discussion_r2624139874
PR Review Comment: https://git.openjdk.org/jdk/pull/28692#discussion_r2624178071
PR Review Comment: https://git.openjdk.org/jdk/pull/28692#discussion_r2624149971
