jdk 8338021: Support new unsigned and saturating vector operators in VectorAPI

trafficstars

Hi All,

As per the discussion on panama-dev mailing list[1], patch adds the support for following new vector operators.

     . SUADD   : Saturating unsigned addition.
     . SADD    : Saturating signed addition. 
     . SUSUB   : Saturating unsigned subtraction.
     . SSUB    : Saturating signed subtraction.
     . UMAX    : Unsigned max
     . UMIN    : Unsigned min.

New vector operators are applicable to only integral types since their values wraparound in over/underflowing scenarios after setting appropriate status flags. For floating point types, as per IEEE 754 specs there are multiple schemes to handler underflow, one of them is gradual underflow which transitions the value to subnormal range. Similarly, overflow implicitly saturates the floating-point value to an Infinite value.

As the name suggests, these are saturating operations, i.e. the result of the computation is strictly capped by lower and upper bounds of the result type and is not wrapped around in underflowing or overflowing scenarios.

Summary of changes:

Java side implementation of new vector operators.
Add new scalar saturating APIs for each of the above saturating vector operator in corresponding primitive box classes, fallback implementation of vector operators is based over it.
C2 compiler IR and inline expander changes.
Optimized x86 backend implementation for new vector operators and their predicated counterparts.
Extends existing VectorAPI Jtreg test suite to cover new operations.

Kindly review and share your feedback.

Best Regards, PS: Intrinsification and auto-vectorization of new core-lib API will be addressed separately in a follow-up patch.

[1] https://mail.openjdk.org/pipermail/panama-dev/2024-May/020408.html

Progress

[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
[x] Change requires CSR request JDK-8338352 to be approved

Issues

JDK-8338021: Support new unsigned and saturating vector operators in VectorAPI (Enhancement - P4)
JDK-8338352: Support new unsigned and saturating vector operators in VectorAPI (CSR)

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer)
Sandhya Viswanathan (@sviswa7 - Reviewer) 🔄 Re-review required (review applies to 195390fe)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20507

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

Using diff file

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

Webrev

Link to Webrev Comment

Aug 08 '24 06:08 jatin-bhateja

:wave: Welcome back jbhateja! 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.

Aug 08 '24 06:08 bridgekeeper[bot]

@jatin-bhateja This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8338021: Support new unsigned and saturating vector operators in VectorAPI

Reviewed-by: psandoz, epeter, sviswanathan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 41 new commits pushed to the master branch:

9f6211bcf1b46e4bfba2d128d9eb8457bc0cde51: 8341371: CDS cannot load archived heap objects with -XX:+UseSerialGC -XX:-UseCompressedOops
120a9357b3cf63427a6c8539128b69b11b9beca3: 8342561: Metaspace for generated reflection classes is no longer needed
d5fb6b4a3cf4926acb333e7ee55f96fc76225631: 8339939: [JVMCI] Don't compress abstract and interface Klasses
a5ad974bec932c63ddc647c9986a513ae32ef663: 8343056: C2: Micro-optimize Node lists grow
ec0618742ff6cfd6d83f1278e8d245673fb9ef2c: 8034066: Incorrect alignment in the "Code" section for "-c -XDdetails" options
eb3669a5869d3066341e63dfb8792bd967663656: 8340796: Use a consistent order when loading cxq and EntryList
0e3fc93dfb14378a848571a6b83282c0c73e690f: 8342083: Make a few fields in FileSystemPreferences final
762a573ef1f4d800b98d3acfcc72c0b2792de69e: 8335880: More troubleshooting tips around windows space in path
40e07a7ea31d04722cda3e6d2fc988df50a7cdca: 8342865: Use type parameter for Class::getPrimitiveClass
9e451aa36586badc7be58804ae6f12e6b671445d: 8343102: Remove --compress from jlink command lines from jpackage tests
... and 31 more: https://git.openjdk.org/jdk/compare/158b93d19a518d2b9d3d185e2d4c4dbff9c82aab...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Aug 08 '24 06:08 openjdk[bot]

@jatin-bhateja The following labels will be automatically applied to this pull request:

core-libs
hotspot

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

Aug 08 '24 06:08 openjdk[bot]

/label add hotspot-compiler-dev

Aug 08 '24 11:08 jatin-bhateja

@jatin-bhateja The hotspot-compiler label was successfully added.

Aug 08 '24 11:08 openjdk[bot]

Webrevs

Aug 08 '24 17:08 mlbridge[bot]

@jatin-bhateja The hotspot-compiler label was already applied.

Aug 08 '24 17:08 openjdk[bot]

/csr needed

Aug 08 '24 19:08 jddarcy

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jatin-bhateja please create a CSR request for issue JDK-8338021 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Aug 08 '24 19:08 openjdk[bot]

Some general impressions of the API change in the java.lang classes. I don't think the change as-is, especially the new constant fields, are a great fit for the current API and I think those constant would look worse in a future where there was an "UnsignedInt" value class, so similar fuller platform support.

Aug 26 '24 22:08 jddarcy

I really like the additions here. More scalar ops and vector ops are fantastic! But I'd like you to split it into scalar and vector changes. Because on both sides we'll have to do some review work to get it all right.

Sep 03 '24 13:09 eme64

You did in fact add java/lang methods. I think you need to add tests for all of those. As well. That's going to be even more code to review.

Sep 03 '24 13:09 eme64

You did in fact add java/lang methods. I think you need to add tests for all of those. As well. That's going to be even more code to review.

Hi @eme64 , As Paul suggested in offline mail chain, lets restrict the changes with this patch to only VectorAPI. Going forward we may need to add special Unsigned value classes wrapping around equivalent sized integers. For the time being moving all the helper APIs int VectorMathUtils.java, these automatically gets exercised by the fallback implementation and we already have tests for new APIs.

Sep 05 '24 07:09 jatin-bhateja

No time to review now. But the title only talks about saturating vector operations. UMin/ UMax is not really a saturating operation, right? Preferably, move it to a separate PR, or at least change the title, please :)

Just note on the length of this PR: people are not really excited to review 9k lines at once. I personally spend quite a bit of effort splitting things into smaller units, so that I get things reviewed quicker, and so that I make the life of the reviewer easier. It would be nice if you could split things into smaller units, I think in the end you would get more reviews quicker, and the result would be of higher quality.

Sep 05 '24 14:09 eme64

/Reviewers 2

Sep 06 '24 22:09 sviswa7

Do we have tests for the publically exposed methods in this new file? Or are they only implicitly tested through the VectorAPI, and its tests? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java

Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

We have sufficient test coverage of these APIs in JTREG tests.

Sep 18 '24 13:09 jatin-bhateja

Do we have tests for the publically exposed methods in this new file? Or are they only implicitly tested through the VectorAPI, and its tests? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

We have sufficient test coverage of these APIs in JTREG tests.

@jatin-bhateja I can't see any dedicated JTREG tests to the VectorMath methods. I only see the VectorAPI tests. Can you point me to the VectorMath tests? I'd like to review them.

Sep 18 '24 14:09 eme64

Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

It's whatever math functions are required to in support of vector operations (as the JavaDoc indicates) that are not provided by other classes such as the boxed primitives or java.lang.Math.

/**
 * The class {@code VectorMath} contains methods for performing
 * scalar numeric operations in support of vector numeric operations.
 */
public final class VectorMath {

These are referenced by the vector operators e.g.,

    /** Produce saturating {@code a+b}.  Integral only.
     * @see VectorMath#addSaturating(int, int)
     */
    public static final Binary SADD = binary("SADD", "+", VectorSupport.VECTOR_OP_SADD, VO_NOFP);

And in addition these methods would be used by any tail computation (and the fallback code).

At the moment we are uncertain whether such operations should reside elsewhere and we did not want to block progress. I am not beholden to the name, but so far i cannot think of a concise alternative.VectorOperatorMath is arguably more precise but more verbose.

Sep 18 '24 16:09 PaulSandoz

Do we have tests for the publically exposed methods in this new file? Or are they only implicitly tested through the VectorAPI, and its tests? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java We have sufficient test coverage of these APIs in JTREG tests.

@jatin-bhateja I can't see any dedicated JTREG tests to the VectorMath methods. I only see the VectorAPI tests. Can you point me to the VectorMath tests? I'd like to review them.

I think Jatin is relying on the vector tests to also test the scalar operations by virtue that eventually the scalar result will be compared with the C2 result. Although both might produced the same result both maybe incorrect! We need some independent scalar tests, especially so if later on these are also made intrinsic. I shall volunteer to add some.

Sep 18 '24 17:09 PaulSandoz

Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

It's whatever math functions are required to in support of vector operations (as the JavaDoc indicates) that are not provided by other classes such as the boxed primitives or java.lang.Math.

Ok. I suppose these methods could eventually be moved to java.lang.Math or some other java.lang class, when the VectorAPI goes out of incubator mode?

I feel like these saturating operations, and also the unsigned ops could find a more wider use, away from (explicit) vector usage. For example, the saturating operations are nice because they prevent overflows, and in some cases that would be very nice to have readily available.

Sep 19 '24 06:09 eme64

Do we have tests for the publically exposed methods in this new file? Or are they only implicitly tested through the VectorAPI, and its tests? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

We have sufficient test coverage of these APIs in JTREG tests.

@jatin-bhateja I can't see any dedicated JTREG tests to the VectorMath methods. I only see the VectorAPI tests. Can you point me to the VectorMath tests? I'd like to review them.

Hi @eme64 , @PaulSandoz
Yes dedicated test for each of newly added VectorMath operation is justified here.

Thanks, let me know if there are other comments.

Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

It's whatever math functions are required to in support of vector operations (as the JavaDoc indicates) that are not provided by other classes such as the boxed primitives or java.lang.Math.

Ok. I suppose these methods could eventually be moved to java.lang.Math or some other java.lang class, when the VectorAPI goes out of incubator mode?

I feel like these saturating operations, and also the unsigned ops could find a more wider use, away from (explicit) vector usage. For example, the saturating operations are nice because they prevent overflows, and in some cases that would be very nice to have readily available.

Hi @eme64 , yes that what our extended plan is, for this patch we want to restrict its use to VectorAPI.

Do we have tests for the publically exposed methods in this new file? Or are they only implicitly tested through the VectorAPI, and its tests? src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

We have sufficient test coverage of these APIs in JTREG tests.

@jatin-bhateja I can't see any dedicated JTREG tests to the VectorMath methods. I only see the VectorAPI tests. Can you point me to the VectorMath tests? I'd like to review them.

@eme64 , @PaulSandoz , I agree that explicit test for all newly added VectorMath operation for all integral types is justified here.

Sep 19 '24 06:09 jatin-bhateja

Why is this even called VectorMath? Because those ops are not at all restricted to vectorization, right?

Nomenclature is suggested by Paul.

@PaulSandoz Do you want to limit these scalar operations to a class name that implies vector use?

It's whatever math functions are required to in support of vector operations (as the JavaDoc indicates) that are not provided by other classes such as the boxed primitives or java.lang.Math.

Ok. I suppose these methods could eventually be moved to java.lang.Math or some other java.lang class, when the VectorAPI goes out of incubator mode?

I feel like these saturating operations, and also the unsigned ops could find a more wider use, away from (explicit) vector usage. For example, the saturating operations are nice because they prevent overflows, and in some cases that would be very nice to have readily available.

Hi @eme64 , as per @PaulSandoz and @jddarcy we should wait till Valhalla preview to add full blown unsigned value type and associated operations, for the time being restricting the scope of these new operations to VectorAPI.

Sep 19 '24 06:09 jatin-bhateja

I sent a pull request to your branch https://github.com/jatin-bhateja/jdk/pull/5/files that moves the VectorMath test to the library area and updates it to be more like a library test.

Sep 24 '24 20:09 PaulSandoz

⚠️ @jatin-bhateja This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Sep 25 '24 04:09 openjdk[bot]

@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

Oct 01 '24 05:10 openjdk[bot]

/reviewers 3

Oct 08 '24 16:10 PaulSandoz

@PaulSandoz The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

Oct 08 '24 16:10 openjdk[bot]

@jatin-bhateja 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-8338201
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

Oct 08 '24 16:10 openjdk[bot]

Hi @vnkozlov , Can you kindly run this through your test infrastructure. We have two review approvals for Java and x86 backend code.

Oct 10 '24 16:10 jatin-bhateja

Hi @vnkozlov , Can you kindly run this through your test infrastructure. We have two review approvals for Java and x86 backend code.

I have kicked off some internal tests (FYI @vnkozlov)

Oct 15 '24 16:10 PaulSandoz

jdk jdk copied to clipboard

8338021: Support new unsigned and saturating vector operators in VectorAPI

Progress

Issues

Reviewers

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard