8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes #20634

sviswa7 · 2024-08-19T21:47:23Z

Currently the rearrange and selectFrom APIs check shuffle indices and throw IndexOutOfBoundsException if there is any exceptional source index in the shuffle. This causes the generated code to be less optimal. This PR modifies the rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes and performs optimizations to generate efficient code.

Summary of changes is as follows:

The rearrange/selectFrom methods do wrapIndexes instead of checkIndexes.
Intrinsic for wrapIndexes and selectFrom to generate efficient code

For the following source:

    public void test() {
        var index = ByteVector.fromArray(bspecies128, shuffles[1], 0);
        for (int j = 0; j < bspecies128.loopBound(size); j += bspecies128.length()) {
            var inpvect = ByteVector.fromArray(bspecies128, byteinp, j);
            index.selectFrom(inpvect).intoArray(byteres, j);
        }
    }

The code generated for inner main now looks as follows:
;; B24: # out( B24 B25 ) <- in( B23 B24 ) Loop( B24-B24 inner main of N173 strip mined) Freq: 4160.96
0x00007f40d02274d0: movslq %ebx,%r13
0x00007f40d02274d3: vmovdqu 0x10(%rsi,%r13,1),%xmm1
0x00007f40d02274da: vpshufb %xmm2,%xmm1,%xmm1
0x00007f40d02274df: vmovdqu %xmm1,0x10(%rax,%r13,1)
0x00007f40d02274e6: vmovdqu 0x20(%rsi,%r13,1),%xmm1
0x00007f40d02274ed: vpshufb %xmm2,%xmm1,%xmm1
0x00007f40d02274f2: vmovdqu %xmm1,0x20(%rax,%r13,1)
0x00007f40d02274f9: vmovdqu 0x30(%rsi,%r13,1),%xmm1
0x00007f40d0227500: vpshufb %xmm2,%xmm1,%xmm1
0x00007f40d0227505: vmovdqu %xmm1,0x30(%rax,%r13,1)
0x00007f40d022750c: vmovdqu 0x40(%rsi,%r13,1),%xmm1
0x00007f40d0227513: vpshufb %xmm2,%xmm1,%xmm1
0x00007f40d0227518: vmovdqu %xmm1,0x40(%rax,%r13,1)
0x00007f40d022751f: add $0x40,%ebx
0x00007f40d0227522: cmp %r8d,%ebx
0x00007f40d0227525: jl 0x00007f40d02274d0

Best Regards,
Sandhya

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8340142 to be approved
Commit message must refer to an issue

Issues

JDK-8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes (Enhancement - P4)
JDK-8340142: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes (CSR)

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer) Review applies to 428f2289
Jatin Bhateja (@jatin-bhateja - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20634

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-19T21:48:09Z

👋 Welcome back sviswanathan! 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 · 2024-08-19T21:48:57Z

@sviswa7 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:

8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes

Reviewed-by: jbhateja, psandoz

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 261 new commits pushed to the master branch:

d2e7708: 8341367: Problemlist ShapeNotSetSometimes.java on macOS
0314973: 8341060: Cleanup statics in HeapDumper
021bf63: 8340458: Open source additional Component tests (part 2)
9a7817b: 8340988: Update jdk/jfr/event/gc/collection tests to accept "CodeCache GC Threshold" as valid GC reason
f2a767f: 8340907: Open source closed frame tests # 2
7b1e6f8: 8337389: Parallel: Remove unnecessary forward declarations in psScavenge.hpp
2120a84: 8341333: [JVMCI] Export JavaThread::_unlocked_inflated_monitor to JVMCI
684d246: 8341242: Shenandoah: LRB node is not matched as GC barrier after JDK-8340183
7cc7c08: 8337493: [JVMCI] Number of libgraal threads might be too low
f7c7958: 8340420: ZGC: Should call vm_shutdown_during_initialization if initialization fails
... and 251 more: https://git.openjdk.org/jdk/compare/81ff91ef27a6a856ae2c453a9a9b8333b91da3ab...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.

openjdk · 2024-08-19T21:49:16Z

@sviswa7 The following labels will be automatically applied to this pull request:

core-libs
graal
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.

PaulSandoz · 2024-08-22T18:21:50Z

API shapes are good!

I see you intrinsified selectFrom which, IIUC, optimally generates C2 nodes that are functionally equivalent to the Java expression v.rearrange(this.toShuffle()). That way we can better generate an optimal set of instructions?

Do you know what deficiencies there that blocks us from compiling the expression down to the same set of instructions as the intrinsic? Not suggesting we do that here, just for future reference.

PaulSandoz · 2024-08-22T18:43:56Z

Adding link to UTF-8 decoding use case for convenience and reminder: https://github.com/AugustNagro/utf8.java/blob/master/src/main/java/com/augustnagro/utf8/Utf8.java.

sviswa7 · 2024-08-22T19:54:35Z

API shapes are good!

I see you intrinsified selectFrom which, IIUC, optimally generates C2 nodes that are functionally equivalent to the Java expression v.rearrange(this.toShuffle()). That way we can better generate an optimal set of instructions?

Do you know what deficiencies there that blocks us from compiling the expression down to the same set of instructions as the intrinsic? Not suggesting we do that here, just for future reference.

Yes, I intrinsified to generate optimial set of instructions. In the expression v.rearrange(this.toShuffle()) we will do first partial wrap as part of this.toShuffle() and then full wrap as part of rearrange. In the intrinsic I am only doing full wrap. Without intrinsic, if for whatever reason the this.toShuffle() is not moved out of the loop by the JIT, we incur additional overhead of the partial wrap in the hot code path.

I saw this happening when the following is run as part of the jmh instead of being called from standalone java with a loop:
var index = ByteVector.fromArray(bspecies128, shuffles[1], 0);
for (int j = 0; j < bspecies128.loopBound(size); j += bspecies128.length()) {
var inpvect = ByteVector.fromArray(bspecies128, byteinp, j);
index.selectFrom(inpvect).intoArray(byteres, j);
}

The perf difference between the intrinsic and no intrinsic observed in this case then is about 20%.

PaulSandoz · 2024-09-12T17:23:27Z

I think this is good enough to promote out of draft and create a CSR for the API changes.

…ngewrap

sviswa7 · 2024-09-12T23:05:52Z

/label hotspot-compiler

openjdk · 2024-09-12T23:06:41Z

@sviswa7
The hotspot-compiler label was successfully added.

mlbridge · 2024-09-12T23:17:13Z

Webrevs

merykitty · 2024-09-13T17:20:40Z

Given rearrange with 1 vector gets wrapping indices semantics. I think we should stop normalizing indices when converting a Vector into a VectorShuffle (currently we wrap all out-of-bound elements to [-VLEN, 0)). Then the rearrange with 2 vectors will also wrap similarly (all indices are & (VLEN * 2 - 1), then indices [0, VLEN) maps to the first vector and indices [VLEN, 2 * VLEN) map to the second vector). We will normalize the indices when we invoke VectorShuffle::toVector which I think is much less used than Vector::toShuffle. What do you think?

src/hotspot/share/opto/vectorIntrinsics.cpp

sviswa7 · 2024-09-13T18:17:21Z

Given rearrange with 1 vector gets wrapping indices semantics. I think we should stop normalizing indices when converting a Vector into a VectorShuffle (currently we wrap all out-of-bound elements to [-VLEN, 0)). Then the rearrange with 2 vectors will also wrap similarly (all indices are & (VLEN * 2 - 1), then indices [0, VLEN) maps to the first vector and indices [VLEN, 2 * VLEN) map to the second vector). We will normalize the indices when we invoke VectorShuffle::toVector which I think is much less used than Vector::toShuffle. What do you think?

The guidance from Paul Sandoz and John Rose is to keep the the partial wrapping at shuffle construction as is for now and only change the rearrange and selectFrom apis.

PaulSandoz · 2024-09-13T19:45:11Z

The guidance from Paul Sandoz and John Rose is to keep the the partial wrapping at shuffle construction as is for now and only change the rearrange and selectFrom apis.

Yes, we are trying to take smaller incremental steps. Once the we are done with this work we can step back and discuss/review what to do about shuffles.

PaulSandoz · 2024-09-13T19:55:44Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java

     *
     * The result is the same as the expression
-     * {@code v.rearrange(this.toShuffle())}.
+     * {@code v.rearrange(this.toShuffle().wrapIndexes())}.


Since we also adjusted rearrange the existing expression is fine, recommend no change here and to the mask accepting version.

PaulSandoz · 2024-09-13T20:03:12Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java

     *
     * For each lane {@code N} of the shuffle, and for each lane
-     * source index {@code I=s.laneSource(N)} in the shuffle,
+     * source index {@code I=s.wrapIndex(s.laneSource(N))} in the shuffle,


The pseudo code below starting at line 2644 needs adjusting to:

Vector<E> r = this.rearrange(s); return broadcast(0).blend(r, m);

PaulSandoz · 2024-09-13T20:05:54Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java

+                   this, ws, m,
                   (v1, s_, m_) -> v1.uOp((i, a) -> {
                        int ei = s_.laneSource(i);
                        return ei < 0  || !m_.laneIsSet(i) ? 0 : v1.lane(ei);


The ei < 0 test is redundant.

sviswa7 · 2024-09-13T22:30:29Z

@PaulSandoz Thanks a lot for the review. I have addressed your review comments.

PaulSandoz

Java changes are good (I created a CSR). The approach in HotSpot looks good to me, but need HotSpot reviewers.

sviswa7 · 2024-09-16T16:59:24Z

@PaulSandoz Thanks a lot for the review and the CSR. I will look forward to Hotspot review and CSR progress/approval.

PaulSandoz · 2024-09-17T18:21:43Z

Adding link to UTF-8 decoding use case for convenience and reminder: https://github.com/AugustNagro/utf8.java/blob/master/src/main/java/com/augustnagro/utf8/Utf8.java.

Another related link to base 64 decoding https://github.com/simdutf/SimdBase64/

sviswa7 · 2024-09-17T18:39:04Z

Adding link to UTF-8 decoding use case for convenience and reminder: https://github.com/AugustNagro/utf8.java/blob/master/src/main/java/com/augustnagro/utf8/Utf8.java.

Another related link to base 64 decoding https://github.com/simdutf/SimdBase64/

Thanks Paul!

eme64

I'm a bit confused by the name shuffleWrapIndexes and inline_vector_shuffle_wrap_indexes.

Are you shuffling wrap-indexes? I don't know what that would even mean. I think you should name it wrapShuffleIndexes. Or is there any naming convention in the VectorAPI that prevents this?

sviswa7 · 2024-09-18T15:27:52Z

I'm a bit confused by the name shuffleWrapIndexes and inline_vector_shuffle_wrap_indexes.

Are you shuffling wrap-indexes? I don't know what that would even mean. I think you should name it wrapShuffleIndexes. Or is there any naming convention in the VectorAPI that prevents this?

Agree, wrapShuffleIndexes makes more sense. I will make the change.

src/hotspot/share/opto/vectorIntrinsics.cpp

jatin-bhateja

Hi @sviswa7 , some comments, overall patch looks good to me.

Best Regards,
Jatin

sviswa7 · 2024-09-19T21:39:43Z

Thanks a lot @jatin-bhateja. I have implemented your review comments.

sviswa7 · 2024-09-19T21:42:47Z

Thanks a lot @eme64 for the review. I have implemented your review comment.

jatin-bhateja

Thanks @sviswa7 , LGTM

sviswa7 · 2024-10-01T22:45:32Z

/integrate

openjdk · 2024-10-01T22:48:32Z

Going to push as commit 83dcb02.
Since your change was applied there have been 261 commits pushed to the master branch:

d2e7708: 8341367: Problemlist ShapeNotSetSometimes.java on macOS
0314973: 8341060: Cleanup statics in HeapDumper
021bf63: 8340458: Open source additional Component tests (part 2)
9a7817b: 8340988: Update jdk/jfr/event/gc/collection tests to accept "CodeCache GC Threshold" as valid GC reason
f2a767f: 8340907: Open source closed frame tests # 2
7b1e6f8: 8337389: Parallel: Remove unnecessary forward declarations in psScavenge.hpp
2120a84: 8341333: [JVMCI] Export JavaThread::_unlocked_inflated_monitor to JVMCI
684d246: 8341242: Shenandoah: LRB node is not matched as GC barrier after JDK-8340183
7cc7c08: 8337493: [JVMCI] Number of libgraal threads might be too low
f7c7958: 8340420: ZGC: Should call vm_shutdown_during_initialization if initialization fails
... and 251 more: https://git.openjdk.org/jdk/compare/81ff91ef27a6a856ae2c453a9a9b8333b91da3ab...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-01T22:48:38Z

@sviswa7 Pushed as commit 83dcb02.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

sviswa7 added 2 commits August 1, 2024 13:14

Wrap shuffle on rearrange

b224593

Optionally partial wrap shuffles during construction

c5c4933

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Aug 19, 2024

sviswa7 mentioned this pull request Aug 19, 2024

8338023: Support two vector selectFrom API #20508

Closed

4 tasks

sviswa7 added 2 commits August 21, 2024 16:58

Initial feedback

d7cd161

Some small fixes

8d73dd6

sviswa7 changed the title ~~Rearrangewrap~~ 8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes Sep 12, 2024

sviswa7 added 2 commits September 12, 2024 15:55

Some cleanup

f67d14a

Merge branch 'master' of https://git.openjdk.java.net/jdk into rearra…

694aceb

…ngewrap

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 12, 2024

sviswa7 marked this pull request as ready for review September 12, 2024 23:13

openjdk bot added the rfr Pull request is ready for review label Sep 12, 2024

jatin-bhateja reviewed Sep 13, 2024

View reviewed changes

src/hotspot/share/opto/vectorIntrinsics.cpp Show resolved Hide resolved

PaulSandoz reviewed Sep 13, 2024

View reviewed changes

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 13, 2024

Address review comments

428f228

PaulSandoz approved these changes Sep 16, 2024

View reviewed changes

eme64 reviewed Sep 18, 2024

View reviewed changes

Change method name

87e103e

PaulSandoz mentioned this pull request Sep 18, 2024

8310691: [REDO] [vectorapi] Refactor VectorShuffle implementation #21042

Closed

3 tasks

jatin-bhateja reviewed Sep 19, 2024

View reviewed changes

Implement review comments

f8e67fb

jatin-bhateja approved these changes Sep 20, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Oct 1, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 1, 2024

openjdk bot closed this Oct 1, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 1, 2024

8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes #20634

8340079: Modify rearrange/selectFrom Vector API methods to perform wrapIndexes instead of checkIndexes #20634

Uh oh!

Conversation

sviswa7 commented Aug 19, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 19, 2024

Uh oh!

openjdk bot commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 19, 2024

Uh oh!

PaulSandoz commented Aug 22, 2024

Uh oh!

PaulSandoz commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sviswa7 commented Aug 22, 2024

Uh oh!

PaulSandoz commented Sep 12, 2024

Uh oh!

sviswa7 commented Sep 12, 2024

Uh oh!

openjdk bot commented Sep 12, 2024

Uh oh!

mlbridge bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

merykitty commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sviswa7 commented Sep 13, 2024

Uh oh!

PaulSandoz commented Sep 13, 2024

Uh oh!

PaulSandoz Sep 13, 2024 • 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!

PaulSandoz Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

PaulSandoz Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

sviswa7 commented Sep 13, 2024

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

sviswa7 commented Sep 16, 2024

Uh oh!

PaulSandoz commented Sep 17, 2024

Uh oh!

sviswa7 commented Sep 17, 2024

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

sviswa7 commented Sep 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sviswa7 commented Aug 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 19, 2024 •

edited

Loading

PaulSandoz commented Aug 22, 2024 •

edited

Loading

mlbridge bot commented Sep 12, 2024 •

edited

Loading

merykitty commented Sep 13, 2024 •

edited

Loading

PaulSandoz Sep 13, 2024 •

edited

Loading