8282764: AArch64: compiler/vectorapi/reshape/TestVectorCastNeon.java failed with incorrect result #7850

nsjian · 2022-03-17T04:33:59Z

Vector API long to float conversion operation on NEON converts vector of 2 longs to 2 floats. In current implementation, we convert 2 longs to 2 doubles first and then converts 2 doubles to 2 floats. However, this two-steps conversion may have two roundings, while the expected behavior, conversion from long to float directly, has only one rounding. This will result in inconsistent result. E.g. for the failure test case:

jshell> long l = 0x561a524000000001L;
l ==> 6204361871487664129

jshell> float l2f = (float)l;
l2f ==> 6.2043621E18

jshell> float l2d2f = (float)((double)l);
l2d2f ==> 6.2043616E18

Since we don't have NEON instruction to support vector long to float conversion, we fix the codegen by doing it in scalar operation one element by one element.

TEST: vector api jtreg tests passed.

Progress

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

Issue

JDK-8282764: AArch64: compiler/vectorapi/reshape/TestVectorCastNeon.java failed with incorrect result

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer)
Nick Gasson (@nick-arm - Reviewer)
Eric Liu - Author ⚠️ Added manually

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7850/head:pull/7850
$ git checkout pull/7850

Update a local copy of the PR:
$ git checkout pull/7850
$ git pull https://git.openjdk.java.net/jdk pull/7850/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7850

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7850.diff

…failed with incorrect result Vector API long to float conversion operation on NEON converts vector of 2 longs to 2 floats. In current implementation, we convert 2 longs to 2 doubles first and then converts 2 doubles to 2 floats. However, this two-steps conversion may have two roundings, while the expected behavior, conversion from long to float directly, has only one rounding. This will result in inconsistent result. E.g. for the failure test case: ``` jshell> long l = 0x561a524000000001L; l ==> 6204361871487664129 jshell> float l2f = (float)l; l2f ==> 6.2043621E18 jshell> float l2d2f = (float)((double)l); l2d2f ==> 6.2043616E18 ``` Since we don't have NEON instruction to support vector long to float conversion, we fix the codegen by doing it in scalar operation one element by one element. TEST: vector api jtreg tests passed.

bridgekeeper · 2022-03-17T04:34:37Z

👋 Welcome back njian! 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 · 2022-03-17T04:36:29Z

@nsjian 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 · 2022-03-17T04:40:10Z

Webrevs

01: Full - Incremental (bf094065)
00: Full (790fa68f)

e1iu · 2022-03-23T01:33:12Z

Looks good to me (not a reviewer).

nsjian · 2022-03-23T02:31:48Z

Looks good to me (not a reviewer).

Thank you @theRealELiu !

PaulSandoz · 2022-03-28T19:19:56Z

test/hotspot/jtreg/compiler/vectorapi/VectorCastShape128Test.java

        0,
        Long.MAX_VALUE,
        Long.MIN_VALUE,
+        0x561a524000000001L,


A comment would be useful as to why this constant is important

PaulSandoz

Thanks for updating the copyright year. Java changes look good. Just one comment related to an additional value added for testing.

nsjian · 2022-03-29T07:39:15Z

Thanks for updating the copyright year. Java changes look good. Just one comment related to an additional value added for testing.

Thank you for the review @PaulSandoz ! Code comment added.

PaulSandoz

Java changes are good.

openjdk · 2022-03-29T17:37:14Z

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

8282764: AArch64: compiler/vectorapi/reshape/TestVectorCastNeon.java failed with incorrect result

Reviewed-by: psandoz, ngasson, 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 168 new commits pushed to the master branch:

b82b009: 8283737: riscv: MacroAssembler::stop() should emit fixed-length instruction sequence
edb42d7: 8282547: IGV: add control-flow graph view
7418373: 8283788: Remove unused VM_DeoptimizeAll::_dependee
aa33525: 8283787: C1: Remove unused ArrayStoreExceptionStub::_info
8b65611: 8283789: CompilerPhaseTypeHelper::to_bitmask should operate on uint64_t
9bb916d: 8283800: Simplify String.indexOf/lastIndexOf calls
b323f54: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
eb5b712: 8283701: Add final or sealed modifier to appropriate java.awt.color ICC_Profile API classes
d066856: 8282162: [vector] Optimize integral vector negation API
bfd9c2b: 8283015: Create a test for JDK-4715496
... and 158 more: https://git.openjdk.java.net/jdk/compare/de4f04cb71a26ce03b96460cb8d1c1e28cd1ed38...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.

nick-arm

cpu/aarch64 changes look OK to me and the Windows x64 failure is some unrelated environment setup problem.

nsjian · 2022-03-30T09:43:29Z

Thanks for the review! @nick-arm

/reviewer credit @theRealELiu

openjdk · 2022-03-30T09:44:21Z

@nsjian
Reviewer eliu successfully credited.

nsjian · 2022-03-30T09:46:01Z

/integrate

openjdk · 2022-03-30T09:47:23Z

Going to push as commit 51c05e8.
Since your change was applied there have been 168 commits pushed to the master branch:

b82b009: 8283737: riscv: MacroAssembler::stop() should emit fixed-length instruction sequence
edb42d7: 8282547: IGV: add control-flow graph view
7418373: 8283788: Remove unused VM_DeoptimizeAll::_dependee
aa33525: 8283787: C1: Remove unused ArrayStoreExceptionStub::_info
8b65611: 8283789: CompilerPhaseTypeHelper::to_bitmask should operate on uint64_t
9bb916d: 8283800: Simplify String.indexOf/lastIndexOf calls
b323f54: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
eb5b712: 8283701: Add final or sealed modifier to appropriate java.awt.color ICC_Profile API classes
d066856: 8282162: [vector] Optimize integral vector negation API
bfd9c2b: 8283015: Create a test for JDK-4715496
... and 158 more: https://git.openjdk.java.net/jdk/compare/de4f04cb71a26ce03b96460cb8d1c1e28cd1ed38...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-30T09:47:42Z

@nsjian Pushed as commit 51c05e8.

💡 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 Mar 17, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 17, 2022

PaulSandoz reviewed Mar 28, 2022

View reviewed changes

Add comment for special value usage.

bf09406

PaulSandoz approved these changes Mar 29, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 29, 2022

nick-arm approved these changes Mar 30, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 30, 2022

openjdk bot closed this Mar 30, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 30, 2022

8282764: AArch64: compiler/vectorapi/reshape/TestVectorCastNeon.java failed with incorrect result #7850

8282764: AArch64: compiler/vectorapi/reshape/TestVectorCastNeon.java failed with incorrect result #7850

Uh oh!

Conversation

nsjian commented Mar 17, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 17, 2022

Uh oh!

openjdk bot commented Mar 17, 2022

Uh oh!

mlbridge bot commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

e1iu commented Mar 23, 2022

Uh oh!

nsjian commented Mar 23, 2022

Uh oh!

PaulSandoz Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

nsjian commented Mar 29, 2022

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nick-arm left a comment

Choose a reason for hiding this comment

Uh oh!

nsjian commented Mar 30, 2022

Uh oh!

openjdk bot commented Mar 30, 2022

Uh oh!

nsjian commented Mar 30, 2022

Uh oh!

openjdk bot commented Mar 30, 2022

Uh oh!

openjdk bot commented Mar 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

nsjian commented Mar 17, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 17, 2022 •

edited

Loading

openjdk bot commented Mar 29, 2022 •

edited

Loading