8256614: AArch64: Add SVE backend implementation for integer min/max

[XiaohongGong]

Currently the Arm SVE implementation for integer (byte,short,int,long) vector min/max is missing. This is needed for VectorAPI. We need to add them all to avoid the "bad AD file" crash.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Testing

	Linux aarch64	Linux arm	Linux ppc64le	Linux s390x	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (1/1 passed)	✔️ (6/6 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)					✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

• JDK-8256614: AArch64: Add SVE backend implementation for integer min/max

Reviewers

• Andrew Dinn (@adinn - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1337/head:pull/1337
$ git checkout pull/1337

[bridgekeeper]

👋 Welcome back xgong! 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]

@XiaohongGong 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]

Webrevs

• 01: Full - Incremental (3eb99c2)
• 00: Full (fa91bc2)

[adinn]

We need to add them all to avoid the "bad AD file" crash.

I'm not clear how this came about. Is this fix needed as a response to another change that caused the crash? Or is the problem that the incomplete implementation was pushed without testing some cases properly?

Is there a before and after test case for the problem this fixes?

[XiaohongGong]

We need to add them all to avoid the "bad AD file" crash.

I'm not clear how this came about. Is this fix needed as a response to another change that caused the crash? Or is the problem that the incomplete implementation was pushed without testing some cases properly?

Is there a before and after test case for the problem this fixes?

Hi @adinn , thanks for looking at this PR. Currently only the VectorAPI can generate the MinVNode/MaxVNode for integer types. I found this issue when I tried to use the related API to do some other investigation work. And actually there is the jtreg tests (eg: https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/incubator/vector/IntMaxVectorTests.java#L2403) for it. I think the reason that we didn't find the issue is the loop count INVOC_COUNT , which is too small that didn't make the method hot enough to be compiled with C2. Currently the value is set to 100:

static final int INVOC_COUNT = Integer.getInteger("jdk.incubator.vector.test.loop-iterations", 100);

When I use a larger loop count like 10000, the test can crash with bad AD file. So this issue can be reproduced by adding -Djdk.incubator.vector.test.loop-iterations=10000 when running the tests.

[adinn]

Hi @XiaohongGong . Thanks for clarifying that. Your code changes look ok to me but it would be good if we could also change the default setting for this loop count to ensure that this case gets tested when SVE hw is present.

iterations=100 appears to be defaulted in file config.sh in the same dir as the test program. Do you know if using iterations=100 is actually triggering C2 compilation for any case (SVE or other, including on Intel)? If not then we really need to increase the default count to a higher value for all cases.

If this is just an SVE-specific thing then we could maybe add some special case processing to the config script to detect an arch where SVE is present and set a higher value.

Perhaps @iwanowww might be able to comment on what would be a suitable setting for this counter.

[XiaohongGong]

Hi @XiaohongGong . Thanks for clarifying that. Your code changes look ok to me but it would be good if we could also change the default setting for this loop count to ensure that this case gets tested when SVE hw is present.

iterations=100 appears to be defaulted in file config.sh in the same dir as the test program. Do you know if using iterations=100 is actually triggering C2 compilation for any case (SVE or other, including on Intel)? If not then we really need to increase the default count to a higher value for all cases.

If this is just an SVE-specific thing then we could maybe add some special case processing to the config script to detect an arch where SVE is present and set a higher value.

Yeah, I agree that it's better to adjust the loop count for jtreg tests if necessary. Do you think it's better to change it with another patch since I think it's not a SVE special case (NEON and X86 have the same issue)? Changing the default loop count for all tests needs more time to run the tests and make the tests easier to time out. And I'v no idea about how to set the suitable value to balance the time and effectiveness. I think some cases can actually trigger the C2 compilation if using default iterations=100. However, it cannot for simple cases like min/max.

[adinn]

Yeah, I agree that it's better to adjust the loop count for jtreg tests if necessary. Do you think it's better to change it with another patch since I think it's not a SVE special case (NEON and X86 have the same issue)? Changing the default loop count for all tests needs more time to run the tests and make the tests easier to time out. And I've no idea about how to set the suitable value to balance the time and effectiveness. I think some cases can actually trigger the C2 compilation if using default iterations=100. However, it cannot for simple cases like min/max.

Yes, please raise a separate patch for the iteration count change. We can discuss the timing vs time-out issue there, including the option of splitting out simple tests that need higher counts. If you can do that then I'm happy with this patch as it is.

[openjdk]

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

8256614: AArch64: Add SVE backend implementation for integer min/max

Reviewed-by: adinn

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

• 372595c: 8256390: ZGC: Relocate in-place instead of having a heap reserve
• 1df94c9: 8256912: Zero builds fail after JDK-8255984
• b3497f9: 8256839: JavaDoc for java.time.Period.negated() method
• b50b99d: 8256825: Cleanup WeakProcessorPhaseTimes
• 47a78e8: 8256829: GNU hash style is not available on MIPS
• 7551c68: 8256675: Zero: purge biased locking support
• d9ae0db: 8256746: gc/CriticalNativeArgs.java fails without -XX:-CriticalJNINatives
• aac5c2a: 8255984: Shenandoah: "adaptive" heuristic is prone to missing load spikes
• fa75ad6: 8256725: Metaspace: better blocktree and binlist asserts
• 9de5d09: 8256585: Remove in-place conversion vector operators from Vector API
• ... and 80 more: https://git.openjdk.java.net/jdk/compare/cfa92a53c00a3e9048786567b02cb06aa6977b5b...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 (@adinn) 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).

[XiaohongGong]

Yeah, I agree that it's better to adjust the loop count for jtreg tests if necessary. Do you think it's better to change it with another patch since I think it's not a SVE special case (NEON and X86 have the same issue)? Changing the default loop count for all tests needs more time to run the tests and make the tests easier to time out. And I've no idea about how to set the suitable value to balance the time and effectiveness. I think some cases can actually trigger the C2 compilation if using default iterations=100. However, it cannot for simple cases like min/max.

Yes, please raise a separate patch for the iteration count change. We can discuss the timing vs time-out issue there, including the option of splitting out simple tests that need higher counts. If you can do that then I'm happy with this patch as it is.

Sure, I will do it later then. Thanks a lot!

[XiaohongGong]

/integrate

[openjdk]

@XiaohongGong
Your change (at version 3eb99c2) is now ready to be sponsored by a Committer.

[nsjian]

/sponsor

[openjdk]

@nsjian @XiaohongGong Since your change was applied there have been 90 commits pushed to the master branch:

• 372595c: 8256390: ZGC: Relocate in-place instead of having a heap reserve
• 1df94c9: 8256912: Zero builds fail after JDK-8255984
• b3497f9: 8256839: JavaDoc for java.time.Period.negated() method
• b50b99d: 8256825: Cleanup WeakProcessorPhaseTimes
• 47a78e8: 8256829: GNU hash style is not available on MIPS
• 7551c68: 8256675: Zero: purge biased locking support
• d9ae0db: 8256746: gc/CriticalNativeArgs.java fails without -XX:-CriticalJNINatives
• aac5c2a: 8255984: Shenandoah: "adaptive" heuristic is prone to missing load spikes
• fa75ad6: 8256725: Metaspace: better blocktree and binlist asserts
• 9de5d09: 8256585: Remove in-place conversion vector operators from Vector API
• ... and 80 more: https://git.openjdk.java.net/jdk/compare/cfa92a53c00a3e9048786567b02cb06aa6977b5b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 67a9590.

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