8309583: AArch64: Optimize firstTrue() when amount of elements < 8 #14373

changpeng1997 · 2023-06-08T02:44:08Z

This patch optimizes VectorMask.firstTrue() on Neon when there are 2 or 4 elements in vector registers.

VectorMask.firstTrue() should return VLEGNTH when vector mask is all false 1. Current implementation uses rbit and then clz 2 to count leading zeros, then uses csel 3 (conditional select) to get the smaller value between VLENGTH and the number of unset lanes to ensure correctness.

This patch sets the 16th or 32nd bit as 1, when there are only 2 or 4 elements in boolean masks, before rbit and clz. With this trick, maximum value calculated in such case will be VLENGTH (2 or 4).

Test:
All vector and vectorapi test passed.

Performance:
The benchmark functions are in MaskQueryOperationsBenchmark.java [4]. This patch also modifies above benchmark to measure mask operations' performance more effectively.

Following data is collected on a 128-bit Neon machine.

Benchmark (inputs) Mode Before After Units
MaskQueryOperationsBenchmark.testFirstTrueInt 1 thrpt 5952.670 7298.491 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueInt 2 thrpt 5951.513 7297.620 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueInt 3 thrpt 5953.048 7298.072 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueLong 1 thrpt 3496.990 4003.188 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueLong 2 thrpt 3497.755 4002.577 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueLong 3 thrpt 3500.085 4002.471 ops/ms

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-8309583: AArch64: Optimize firstTrue() when amount of elements < 8 (Enhancement - P4)

Reviewers

Andrew Haley (@theRealAph - Reviewer) ⚠️ Review applies to 62a6522c
Eric Liu (@e1iu - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14373

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

Using diff file

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

Webrev

Link to Webrev Comment

@benchmark

This patch optimizes VectorMask.firstTrue() on Neon when there are 2 or 4 elements in vector registers. VectorMask.firstTrue() should return VLEGNTH when vector mask is all false [1]. Current implementation uses rbit and then clz [2] to count leading zeros, then uses csel [3] (conditional select) to get the smaller value between VLENGTH and the number of unset lanes to ensure correctness. This patch sets the 16th or 32nd bit as 1, when there are only 2 or 4 elements in boolean masks, before rbit and clz. With this trick, maximum value calculated in such case will be VLENGTH (2 or 4). Test: All vector and vectorapi test passed. Performance: The benchmark function is like: ``` @benchmark public static int testInt() { int res = 0; for (int i = 0; i < LENGTH; i += INT_SPECIES.length()) { VectorMask<Integer> m = VectorMask.fromArray(INT_SPECIES, ia, i); res += m.firstTrue(); } return res; } ``` Following data is collected on a 128-bit Neon machine. Benchmark Before After Unit testInt 22214.740 25627.833 ops/ms testLong 11649.898 13698.535 ops/ms [1]: https://docs.oracle.com/en/java/javase/20/docs/api/jdk.incubator.vector/jdk/incubator/vector/VectorMask.html#firstTrue() [2]: https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/aarch64_vector.ad#L5540 [3]: https://developer.arm.com/documentation/ddi0602/2021-12/Base-Instructions/CSEL--Conditional-Select- Change-Id: I4a2de805ffa4469f88d510c96617eae165f0e025

bridgekeeper · 2023-06-08T02:45:33Z

👋 Welcome back changpeng1997! 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 · 2023-06-08T02:47:37Z

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

hotspot-compiler

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 · 2023-06-08T02:52:18Z

Webrevs

theRealAph · 2023-06-08T09:15:27Z

Where is the benchmark? You don't seem to have included it in this PR.

changpeng1997 · 2023-06-19T07:53:23Z

Where is the benchmark? You don't seem to have included it in this PR.

@theRealAph

Sorry for the delay. Original performance was measured by a simple benchmark only measuring firstTrue()'s performance written by myself. When I wanted to add it to JDK I found an existing benchmark used to measure different mask operations' performance (jdk/test/micro/org/openjdk/bench/jdk/incubator/vector/MaskQueryOperationsBenchmark.java at master · openjdk/jdk · GitHub). I tried to measure firstTrue()'s performance by this benchmark, but I found Blackhole‘s proportion of hottest region is too high, like following:

[Hottest Methods (after inlining)].............................................................. 
57.15% c2, level 4 org.openjdk.jmh.infra.Blackhole::consumeFull, version 867 
41.99% c2, level 4 org.openjdk.bench.jdk.incubator.vector.jmh_generated.MaskQueryOperationsBenchmark_testFirstTrueLong_jmhTest::testFirstTrueLong_thrpt_jmhStub, version 883.

So I spent some time on fixing this benchmark to measure mask operations' performance effectively. After this update, the proportion of blackhole is below 10% for each benchmark function. And I also updated the performance of firstTrue() measured by this benchmark when there are only 2 or 4 elements in boolean masks.

theRealAph · 2023-06-19T09:52:22Z

Sorry for the delay. Original performance was measured by a simple benchmark only measuring firstTrue()'s performance written by myself. When I wanted to add it to JDK I found an existing benchmark used to measure different mask operations' performance (jdk/test/micro/org/openjdk/bench/jdk/incubator/vector/MaskQueryOperationsBenchmark.java at master · openjdk/jdk · GitHub). I tried to measure firstTrue()'s performance by this benchmark, but I found Blackhole‘s proportion of hottest region is too high, like following:

Can you please send the entire output of JMH? Blackhole should not appear at all in the output because it's been intrinsified. I'd like to know why the intrinsic isn't working for you.

changpeng1997 · 2023-06-19T10:05:37Z

Sorry for the delay. Original performance was measured by a simple benchmark only measuring firstTrue()'s performance written by myself. When I wanted to add it to JDK I found an existing benchmark used to measure different mask operations' performance (jdk/test/micro/org/openjdk/bench/jdk/incubator/vector/MaskQueryOperationsBenchmark.java at master · openjdk/jdk · GitHub). I tried to measure firstTrue()'s performance by this benchmark, but I found Blackhole‘s proportion of hottest region is too high, like following:

Can you please send the entire output of JMH? Blackhole should not appear at all in the output because it's been intrinsified. I'd like to know why the intrinsic isn't working for you.

Output before this patch: https://gist.github.com/changpeng1997/734aa176577bfff56f5a87db9c8db69a
Output after this patch: https://gist.github.com/changpeng1997/73098069b8f814310d6606dfd7dc56c5

theRealAph · 2023-06-19T10:07:55Z

Blackhole mode: full + dont-inline hint (default, use -Djmh.blackhole.autoDetect=true to auto-detect)

Could you ty this?

theRealAph · 2023-06-21T14:25:57Z

Something is wrong with your setup.
You should be seeing this:
# Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable)

not this:
# Blackhole mode: full + dont-inline hint (default, use -Djmh.blackhole.autoDetect=true to auto-detect)

shipilev · 2023-06-21T14:49:08Z

Output before this patch: https://gist.github.com/changpeng1997/734aa176577bfff56f5a87db9c8db69a
Output after this patch: https://gist.github.com/changpeng1997/73098069b8f814310d6606dfd7dc56c5

Blackhole mode autodetection was added in JMH 1.33, and enabled in JMH 1.34. The logs above say they run with JMH 1.33. Current version is 1.36, you need to upgrade, @changpeng1997.

Also, I notice that your before/after logs use different JVM modes, one uses release, and another uses fastdebug. These are not comparable.

theRealAph · 2023-06-21T15:03:35Z

So I'm looking at the results of the patch and I see:

Before

Benchmark                                           (inputs)  Mode  Cnt   Score   Error  Units
MaskQueryOperationsBenchmark.testFirstTrueInt              1  avgt    3  69.547 ± 2.837  ns/op
MaskQueryOperationsBenchmark.testFirstTrueInt              2  avgt    3  69.549 ± 0.497  ns/op
MaskQueryOperationsBenchmark.testFirstTrueInt              3  avgt    3  69.506 ± 1.360  ns/op

After:

Benchmark                                           (inputs)  Mode  Cnt   Score   Error  Units
MaskQueryOperationsBenchmark.testFirstTrueInt              1  avgt    3  58.955 ± 0.838  ns/op
MaskQueryOperationsBenchmark.testFirstTrueInt              2  avgt    3  58.690 ± 2.940  ns/op
MaskQueryOperationsBenchmark.testFirstTrueInt              3  avgt    3  58.923 ± 1.088  ns/op

which corresponds with a change from

            0x00000001158ef748:   fmov	x11, d16
            0x00000001158ef74c:   rbit	x11, x11
            0x00000001158ef750:   clz	x11, x11
            0x00000001158ef754:   lsr	w11, w11, #3
           ;; 0x4
            0x00000001158ef758:   orr	w8, wzr, #0x4
            0x00000001158ef75c:   cmp	w11, w8
            0x00000001158ef760:   csel	w11, w8, w11, ge  // ge = tcont

to

            0x0000000115f3f8e8:   fmov	x14, d16
            0x0000000115f3f8ec:   orr	x14, x14, #0x100000000
            0x0000000115f3f8f0:   rbit	x14, x14
            0x0000000115f3f8f4:   clz	x14, x14
            0x0000000115f3f8f8:   lsr	w14, w14, #3

That's a pretty decent speedup when you consider that the benchmark is dominated by memory operations and vector->core register moves.

theRealAph · 2023-06-21T16:50:14Z

If we care about memory ops, note that we can get a useful speedup with match(Set dst (VectorMaskFirstTrue (LoadVector mem))) but perhaps that's not worth doing.

Benchmark                                           (inputs)  Mode  Cnt   Score   Error  Units
MaskQueryOperationsBenchmark.testFirstTrueInt              1  avgt    3  49.591 ± 0.477  ns/op

I will say that in general if you have to work in the core integer processor on an in-memory vector , it might be worth loading straight into core registers rather than going via the SIMD regs. Maybe we should write a general-purpose function that bypasses the SIMD unit in all such cases.

theRealAph

OK.

openjdk · 2023-06-22T12:49:11Z

⚠️ @changpeng1997 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout optimize_firsttrue2e4e_neon
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2023-06-22T12:49:12Z

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

8309583: AArch64: Optimize firstTrue() when amount of elements < 8

Reviewed-by: aph, eliu

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

8242c64: 8310863: Build failure after JDK- 8305341
78c3831: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes
c92b049: 8308780: Fix the Java Integer types on Windows
0db63ec: 8310571: Use inline @return tag on java.util.Objects
dad7bd9: 8310234: Refactor Locale tests to use JUnit
69f3114: 8310676: add note about unnamed module to Elements.getAllModuleElements
bfcca5e: 8310494: Do not include constantPool.hpp from instanceKlass.hpp
0314292: 8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives
a9c0a0f: 8310628: GcInfoBuilder.c missing JNI Exception checks
556831d: 8310460: Remove jdeps -profile option
... and 72 more: https://git.openjdk.org/jdk/compare/959a61fdd483c9523764b9ba0972f59ca06db0ee...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@theRealAph, @e1iu) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

changpeng1997 · 2023-06-25T02:44:51Z

Something is wrong with your setup. You should be seeing this: # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable)

not this: # Blackhole mode: full + dont-inline hint (default, use -Djmh.blackhole.autoDetect=true to auto-detect)

@theRealAph Sorry for the delay, I was on holiday last week.

I found that we can avoid the effects of blackhole by using -Djmh.blackhole.autoDetect=true, so I have reset this benchmark.

Following is the performance of testFirstTrueInt and testFirstTrueLong before and after this patch:

Benchmark                                       bits    (inputs)  Mode  Before      After      Units
MaskQueryOperationsBenchmark.testFirstTrueInt   128           1  thrpt  520650.354  580091.081 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueInt   128           2  thrpt  520677.937  580391.661 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueInt   128           3  thrpt  519967.269  580705.642 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueLong  128           1  thrpt  518563.126  575941.490 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueLong  128           2  thrpt  517329.190  578848.383 ops/ms
MaskQueryOperationsBenchmark.testFirstTrueLong  128           3  thrpt  517987.339  577601.752 ops/ms

And following are the corresponding JMH output:
before my patch: https://gist.github.com/changpeng1997/3ebe4b7beea93716d9f29d4ef71641af
after my patch: https://gist.github.com/changpeng1997/d306957370eb0bdbb8e71b601440cdaa

We can see the C2 code of firstTrue().

changpeng1997 · 2023-06-25T02:45:57Z

Output before this patch: https://gist.github.com/changpeng1997/734aa176577bfff56f5a87db9c8db69a
Output after this patch: https://gist.github.com/changpeng1997/73098069b8f814310d6606dfd7dc56c5

Blackhole mode autodetection was added in JMH 1.33, and enabled in JMH 1.34. The logs above say they run with JMH 1.33. Current version is 1.36, you need to upgrade, @changpeng1997.

Also, I notice that your before/after logs use different JVM modes, one uses release, and another uses fastdebug. These are not comparable.

@shipilev Thanks! Sorry for this mistake.

changpeng1997 · 2023-06-26T06:06:00Z

/integrate

openjdk · 2023-06-26T06:07:57Z

@changpeng1997
Your change (at version 8ce4ba8) is now ready to be sponsored by a Committer.

e1iu · 2023-06-27T04:08:59Z

/sponsor

openjdk · 2023-06-27T04:10:39Z

Going to push as commit 45b581b.
Since your change was applied there have been 107 commits pushed to the master branch:

87e6fab: 8310873: Re-enable locked_create_entry symbol check in runtime/NMT/CheckForProperDetailStackTrace.java for RISC-V
39fa4e6: 8310489: New test runtime/ClassInitErrors/TestStackOverflowDuringInit.java failed
46add3f: 8310909: java.io.InvalidObjectException has redundant @since tag
8f5b677: 8310908: Non-standard @since tag in com.sun.java.accessibility.util.package-info
a197ee7: 8310838: Correct range notations in MethodTypeDesc specification
7c6a28f: 8310922: java/lang/Class/forName/ForNameNames.java fails after being added by JDK-8310242
7db2f08: 8310242: Clarify the name parameter to Class::forName
297c799: 8301492: Modernize equals() method of ResourceBundle.CacheKey and Bundles.CacheKey
a08352f: 8305671: javac rejects semicolons in compilation units with no imports
ff9a754: 8310459: [BACKOUT] 8304450: [vectorapi] Refactor VectorShuffle implementation
... and 97 more: https://git.openjdk.org/jdk/compare/959a61fdd483c9523764b9ba0972f59ca06db0ee...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-27T04:10:46Z

@e1iu @changpeng1997 Pushed as commit 45b581b.

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

openjdk bot added the rfr Pull request is ready for review label Jun 8, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 8, 2023

changpeng1997 and others added 2 commits June 19, 2023 09:40

Merge branch 'openjdk:master' into optimize_firsttrue2e4e_neon

d850710

Update MaskQueryOperationsBenchmark.java

62a6522

theRealAph approved these changes Jun 22, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 22, 2023

Reset MaskQueryOperationsBenchmark.java

8ce4ba8

e1iu approved these changes Jun 26, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 26, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 27, 2023

openjdk bot closed this Jun 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8309583: AArch64: Optimize firstTrue() when amount of elements < 8 #14373

8309583: AArch64: Optimize firstTrue() when amount of elements < 8 #14373

changpeng1997 commented Jun 8, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 8, 2023

openjdk bot commented Jun 8, 2023

mlbridge bot commented Jun 8, 2023 •

edited

Loading

theRealAph commented Jun 8, 2023

changpeng1997 commented Jun 19, 2023 •

edited

Loading

theRealAph commented Jun 19, 2023

changpeng1997 commented Jun 19, 2023

theRealAph commented Jun 19, 2023

theRealAph commented Jun 21, 2023 •

edited

Loading

shipilev commented Jun 21, 2023

theRealAph commented Jun 21, 2023

theRealAph commented Jun 21, 2023

theRealAph left a comment

openjdk bot commented Jun 22, 2023

openjdk bot commented Jun 22, 2023 •

edited

Loading

changpeng1997 commented Jun 25, 2023

changpeng1997 commented Jun 25, 2023

changpeng1997 commented Jun 26, 2023

openjdk bot commented Jun 26, 2023

e1iu commented Jun 27, 2023

openjdk bot commented Jun 27, 2023

openjdk bot commented Jun 27, 2023

8309583: AArch64: Optimize firstTrue() when amount of elements < 8 #14373

8309583: AArch64: Optimize firstTrue() when amount of elements < 8 #14373

Conversation

changpeng1997 commented Jun 8, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 8, 2023

openjdk bot commented Jun 8, 2023

mlbridge bot commented Jun 8, 2023 • edited Loading

Webrevs

theRealAph commented Jun 8, 2023

changpeng1997 commented Jun 19, 2023 • edited Loading

theRealAph commented Jun 19, 2023

changpeng1997 commented Jun 19, 2023

theRealAph commented Jun 19, 2023

theRealAph commented Jun 21, 2023 • edited Loading

shipilev commented Jun 21, 2023

theRealAph commented Jun 21, 2023

theRealAph commented Jun 21, 2023

theRealAph left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 22, 2023

openjdk bot commented Jun 22, 2023 • edited Loading

changpeng1997 commented Jun 25, 2023

changpeng1997 commented Jun 25, 2023

changpeng1997 commented Jun 26, 2023

openjdk bot commented Jun 26, 2023

e1iu commented Jun 27, 2023

openjdk bot commented Jun 27, 2023

openjdk bot commented Jun 27, 2023

changpeng1997 commented Jun 8, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 8, 2023 •

edited

Loading

changpeng1997 commented Jun 19, 2023 •

edited

Loading

theRealAph commented Jun 21, 2023 •

edited

Loading

openjdk bot commented Jun 22, 2023 •

edited

Loading