8255210: [Vector API] jdk/incubator/vector/Int256VectorTests.java crashes on AVX512 machines #791

DamonFool · 2020-10-22T03:12:31Z

Hi all,

Please review the fix of an AVX512 crash for Vector API.
The reason is that reductionI in x86.ad didn't use legVec for code generation, which is required by Assembler::vphaddd.

Testing:

test/jdk/jdk/incubator/vector all passed on both AVX256 and AVX512 machines

Thanks.
Best regards,
Jie

Progress

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

Testing

	Linux x32	Linux x64	Windows x64	macOS x64
Build	✔️ (1/1 passed)	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)		✔️ (9/9 passed)	❌ (1/9 failed)	✔️ (9/9 passed)

Failed test task

Windows x64 (hs/tier1 compiler)

Issue

JDK-8255210: [Vector API] jdk/incubator/vector/Int256VectorTests.java crashes on AVX512 machines

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer)
sviswanathan - Committer ⚠️ Added manually
jbhateja - Committer ⚠️ Added manually

Download

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

…shes on AVX512 machines

bridgekeeper · 2020-10-22T03:13:27Z

👋 Welcome back jiefu! 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.

DamonFool · 2020-10-22T03:13:44Z

/issue add JDK-8255210
/test
/label add hotspot-compiler
/cc hotspot-compiler

openjdk · 2020-10-22T03:15:10Z

@DamonFool This issue is referenced in the PR title - it will now be updated.

openjdk · 2020-10-22T03:15:21Z

@DamonFool
The hotspot-compiler label was successfully added.

openjdk · 2020-10-22T03:16:14Z

@DamonFool The hotspot-compiler label was already applied.

mlbridge · 2020-10-22T03:19:16Z

Webrevs

00: Full (ca83af3)

DamonFool · 2020-10-22T05:07:14Z

/test

openjdk · 2020-10-22T05:07:50Z

@DamonFool you need to get approval to run the tests in tier1 for commits up until ca83af3

DamonFool · 2020-10-22T11:11:36Z

Hi @DamonFool , similar problem also exists for reduction patterns for other primitive types (Long/Float/Double).

Hi @jatin-bhateja ,

Thanks for your reminder.

I've also noticed these problems.
But I don't have a reproducer right now.
And I need some time to construct one.

So I plan to file another bug to fix them when the reproducer is ready.

Thanks.

PaulSandoz

LGTM based on same changes made previously for short vectors.
Good if Sandhya (@sviswa7) et. al. can review also.

openjdk · 2020-10-22T16:50:05Z

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

8255210: [Vector API] jdk/incubator/vector/Int256VectorTests.java crashes on AVX512 machines

Reviewed-by: psandoz, sviswanathan, jbhateja

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

d8d9197: 8242559: Clean the "libawt_xawt" library from code for headless mode
ff5f226: 8255086: Update the root locale display names
8afdcae: 8255031: Update java/util/prefs/AddNodeChangeListener.java to report more failure info
0aa3c92: 8255262: Remove use of legacy custom @SPEC tag
a0b687b: 8254854: [cgroups v1] Metric limits not properly detected on some join controller combinations
f279ddf: 8248411: [aarch64] Insufficient error handling when CodeBuffer is exhausted
4634dbe: 8223312: Utilize handshakes instead of is_thread_fully_suspended
cc50c8d: 8255196: Remove unused G1FullGCCompactionPoint::merge()
ae72b52: 8255047: Add HotSpot UseDebuggerErgo flags
211bb62: 8255124: KeepAliveStreamCleaner may crash with java.lang.IllegalMonitorStateException: current thread is not owner
... and 4 more: https://git.openjdk.java.net/jdk/compare/5d262290c4994575cbaa76ea58d08e22c3381df8...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.

sviswa7 · 2020-10-22T21:54:03Z

The fix looks good to me as well.

jatin-bhateja · 2020-10-23T02:51:46Z

Hi @DamonFool , similar problem also exists for reduction patterns for other primitive types (Long/Float/Double).

Hi @jatin-bhateja ,

Thanks for your reminder.

I've also noticed these problems.
But I don't have a reproducer right now.
And I need some time to construct one.

So I plan to file another bug to fix them when the reproducer is ready.

Thanks.

Sure , this fix looks good.

DamonFool · 2020-10-23T03:37:09Z

/reviewer credit @sviswa7 , @jatin-bhateja

openjdk · 2020-10-23T03:38:56Z

@DamonFool
Reviewer sviswanathan successfully credited.

Reviewer jbhateja successfully credited.

DamonFool · 2020-10-23T03:43:47Z

Thanks @PaulSandoz , @sviswa7 and @jatin-bhateja for your review.
Will push it later.

DamonFool · 2020-10-23T05:49:25Z

/integrate

openjdk · 2020-10-23T05:53:36Z

@DamonFool Since your change was applied there have been 15 commits pushed to the master branch:

2ca7a08: 8255268: 32-bit failures in runtime/Metaspace/elastic
d8d9197: 8242559: Clean the "libawt_xawt" library from code for headless mode
ff5f226: 8255086: Update the root locale display names
8afdcae: 8255031: Update java/util/prefs/AddNodeChangeListener.java to report more failure info
0aa3c92: 8255262: Remove use of legacy custom @SPEC tag
a0b687b: 8254854: [cgroups v1] Metric limits not properly detected on some join controller combinations
f279ddf: 8248411: [aarch64] Insufficient error handling when CodeBuffer is exhausted
4634dbe: 8223312: Utilize handshakes instead of is_thread_fully_suspended
cc50c8d: 8255196: Remove unused G1FullGCCompactionPoint::merge()
ae72b52: 8255047: Add HotSpot UseDebuggerErgo flags
... and 5 more: https://git.openjdk.java.net/jdk/compare/5d262290c4994575cbaa76ea58d08e22c3381df8...master

Your commit was automatically rebased without conflicts.

Pushed as commit a824781.

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

iwanowww · 2020-10-23T07:26:45Z

src/hotspot/cpu/x86/x86.ad

  predicate(vector_element_basic_type(n->in(2)) == T_INT &&
-            vector_length(n->in(2)) < 16); // src2
+            vector_length(n->in(2)) <= 16); // src2


FTR since reduction16I is gone, vector length check becomes redundant: reductionI successfully covers the whole range of vectors of int.

Nice catch!

The same case with reductionS.

I'll fix it next time.
Thanks.

8255210: [Vector API] jdk/incubator/vector/Int256VectorTests.java cra…

ca83af3

…shes on AVX512 machines

openjdk bot added the rfr Pull request is ready for review label Oct 22, 2020

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 22, 2020

openjdk bot added the test-request label Oct 22, 2020

PaulSandoz approved these changes Oct 22, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2020

openjdk bot closed this Oct 23, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 23, 2020

DamonFool deleted the JDK-8255210 branch October 23, 2020 05:51

iwanowww reviewed Oct 23, 2020

View reviewed changes

DamonFool mentioned this pull request Oct 25, 2020

8255378: [Vector API] Remove redundant vector length check after JDK-8254814 and JDK-8255210 #854

Closed

3 tasks

DamonFool mentioned this pull request Oct 27, 2020

8255438: [Vector API] More instructs in x86.ad should use legacy mode for code-gen #874

Closed

3 tasks

8255210: [Vector API] jdk/incubator/vector/Int256VectorTests.java crashes on AVX512 machines #791

8255210: [Vector API] jdk/incubator/vector/Int256VectorTests.java crashes on AVX512 machines #791

Uh oh!

Conversation

DamonFool commented Oct 22, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Testing

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Oct 22, 2020

Uh oh!

DamonFool commented Oct 22, 2020

Uh oh!

openjdk bot commented Oct 22, 2020

Uh oh!

openjdk bot commented Oct 22, 2020

Uh oh!

openjdk bot commented Oct 22, 2020

Uh oh!

mlbridge bot commented Oct 22, 2020

Webrevs

Uh oh!

DamonFool commented Oct 22, 2020

Uh oh!

openjdk bot commented Oct 22, 2020

Uh oh!

DamonFool commented Oct 22, 2020

Uh oh!

PaulSandoz left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sviswa7 commented Oct 22, 2020

Uh oh!

jatin-bhateja commented Oct 23, 2020

Uh oh!

DamonFool commented Oct 23, 2020

Uh oh!

openjdk bot commented Oct 23, 2020

Uh oh!

DamonFool commented Oct 23, 2020

Uh oh!

DamonFool commented Oct 23, 2020

Uh oh!

openjdk bot commented Oct 23, 2020

Uh oh!

iwanowww Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

DamonFool Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DamonFool commented Oct 22, 2020 •

edited by openjdk bot

Loading

openjdk bot commented Oct 22, 2020 •

edited

Loading