8372978: [VectorAPI] Fix incorrect identity values in UMIN/UMAX reductions #28692

erifan · 2025-12-08T03:22:31Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8372978: [VectorAPI] Fix incorrect identity values in UMIN/UMAX reductions (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28692/head:pull/28692
$ git checkout pull/28692

Update a local copy of the PR:
$ git checkout pull/28692
$ git pull https://git.openjdk.org/jdk.git pull/28692/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28692

View PR using the GUI difftool:
$ git pr show -t 28692

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28692.diff

Using Webrev

Link to Webrev Comment

…tions 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.

bridgekeeper · 2025-12-08T03:23:29Z

👋 Welcome back erfang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-12-08T03:23:35Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-12-08T03:24:27Z

@erifan The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-12-08T03:28:29Z

Webrevs

merykitty · 2025-12-08T05:15:24Z

test/jdk/jdk/incubator/vector/Byte128VectorTests.java

@@ -4189,7 +4189,7 @@ static void MAXReduceByte128VectorTestsMasked(IntFunction<byte[]> fa, IntFunctio
    }

    static byte UMINReduce(byte[] a, int idx) {


We should test the reduction operations in a better manner by using a[idx] as the starting value instead.

Hi @merykitty thanks for the review, a[idx] is already the starting value for the reduction operation of this function, see line 4193. What do you mean by a better manner ?

No, the starting value is -1, this test depends on the fact that we choose the correct identity value for this particular operation as the starting value, which this issue is about. As a result, it would be better to write the test so that we do not depend on the identity value.

byte res = a[idx]; for (int i = idx + 1; i < (idx + SPECIES.length()); i++) { res = (byte) VectorMath.minUnsigned(res, a[i]); }

Okay, I got your point. I think this might be to maintain consistency with UMINReduceMasked; for the masked version, if no element is selected, it returns the identity value. I'm okay with both approaches, maybe let’s hear what @PaulSandoz thinks.

For masking we need to start with the identity or otherwise use the identify when no mask bits are set. It would be better to declare as constants and refer to them e.g., UMAX_VALUE, UMIN_VALUE. There are also other cases where we use identity values for reduction and they follow the same pattern of declaration and use.

Okay, I understand, thank you for your insight! I'll wait for @PaulSandoz 's comment and see if we should add two public constants to this PR. Then I'll modify it accordingly.

I would prefer to declare identity constants, but only in tests [1] , such as MUL_IDENTITY, UMAX_IDENTITY etc, that we consistently refer to and then add basic tests to verify that identity with respect to the scalar operation. The identity values are also embedded in the JDK and HotSpot compiler and i want to ensure they are clearly compared against the expected identity when an all-false mask is used.

[1] later we could place these in some internal JDK class so we can use the same values in the JDK code, then adjust the tests to grant access to the internal JDK class to use those values. A more general place to surface up scalar identity values is in VectorOperators for the associative operators, something to consider later on perhaps, and that would require a CSR.

@PaulSandoz Thanks for your suggestion! I declared some identity constants in both the tests and the implementations. And added some tests to verify the correctness of these constants.

@merykitty Now we're using a correct constant to represent the identity value, eliminating the dependency on incorrect literals. So I've chosen to keep the current coding style. I tried the style you suggested, but I feel the original style is more readable and maintainable. Do you think this is okay?

Please help take another look, thank you!

Then, please add tests to verify the correctness of these identity value (i.e. x + i == x).

Yeah, I have added two tests for these constants, see https://github.com/openjdk/jdk/pull/28692/changes#diff-18d929c1615a4dbad7c65daa66edc98fa1537968fa14a2b8105145d18e9eb12fR6471.

openjdk · 2025-12-12T08:38:24Z

@erifan this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8372978-fix-umin-umax-identity
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

erifan · 2025-12-12T08:52:50Z

Since this issue https://bugs.openjdk.org/browse/JDK-8373390, I didn't merge the latest master branch into this PR. After that issue is fixed, I'll resolve the merge conflicts.

erifan · 2025-12-16T03:32:22Z

Hi, could you please take another look at this PR? I've already processed all your comments. @merykitty @PaulSandoz
And @XiaohongGong @shqking

PaulSandoz · 2025-12-16T17:26:57Z

test/jdk/jdk/incubator/vector/templates/Kernel-Reduction-Masked-op-func.template

+                r[i] = av.reduceLanes(VectorOperators.[[TEST]], vmask);
+                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);

PaulSandoz · 2025-12-16T17:30:25Z

test/jdk/jdk/incubator/vector/templates/Unit-Identity-test.template

+    @Test(dataProvider = "$type$UnaryOpProvider")
+    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

PaulSandoz · 2025-12-16T17:40:02Z

test/jdk/jdk/incubator/vector/templates/Unit-Identity-test.template

+    @Test(dataProvider = "$type$UnaryOpProvider")
+    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?

erifan · 2025-12-17T10:19:06Z

@merykitty @PaulSandoz thanks for your feedbacks, I will make the necessary revisions based on your suggestions shortly. Please continue to review the PR if you have any further suggestions. Thank you!

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 8, 2025

openjdk bot added the rfr Pull request is ready for review label Dec 8, 2025

merykitty reviewed Dec 8, 2025

View reviewed changes

XiaohongGong mentioned this pull request Dec 11, 2025

8372980: [VectorAPI] AArch64: Add intrinsic support for unsigned min/max reduction operations #28693

Draft

3 tasks

erifan added 2 commits December 12, 2025 07:52

Merge branch 'master' into JDK-8372978-fix-umin-umax-identity

fbfcf6a

Declare two constants for UMIN/UMAX reduction identity values

7f73745

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 12, 2025

Merge branch 'master' into JDK-8372978-fix-umin-umax-identity

d685d3d

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 16, 2025

PaulSandoz reviewed Dec 16, 2025

View reviewed changes

		@@ -4189,7 +4189,7 @@ static void MAXReduceByte128VectorTestsMasked(IntFunction<byte[]> fa, IntFunctio
		}

		static byte UMINReduce(byte[] a, int idx) {

		r[i] = av.reduceLanes(VectorOperators.[[TEST]], vmask);
		ra = [[TEST_OP]](ra, r[i]);

		@Test(dataProvider = "$type$UnaryOpProvider")
		static void testMaskedReductionIdentityAllFalse(IntFunction<$type$[]> fa) {

		@Test(dataProvider = "$type$UnaryOpProvider")
		static void testIdentityValues(IntFunction<$type$[]> fa) {

8372978: [VectorAPI] Fix incorrect identity values in UMIN/UMAX reductions #28692

Are you sure you want to change the base?

8372978: [VectorAPI] Fix incorrect identity values in UMIN/UMAX reductions #28692

Uh oh!

Conversation

erifan commented Dec 8, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Solution:

Changes:

Testing:

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Dec 8, 2025

Uh oh!

openjdk bot commented Dec 8, 2025

Uh oh!

openjdk bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merykitty Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 12, 2025

Uh oh!

erifan commented Dec 12, 2025

Uh oh!

erifan commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erifan commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

erifan commented Dec 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 8, 2025 •

edited

Loading

mlbridge bot commented Dec 8, 2025 •

edited

Loading

merykitty Dec 8, 2025 •

edited

Loading