Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8251994: VM crashed running TestComplexAddrExpr.java test with -XX:UseAVX=X #859

Closed
wants to merge 2 commits into from

Conversation

vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Oct 26, 2020

To improve a chance to vectorize a loop, a special code in superword tries to hoist loads to the beginning of loop by replacing their memory input with corresponding (same memory slice) loop's memory Phi :
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/superword.cpp#L474

Originally loads are ordered by corresponding stores on the same memory slice. But when they are hoisted they loose that ordering - nothing enforce the order.
In TestComplexAddrExpr.test6 case the ordering is preserved (luckily?) after hoisting only when vector size is 32 bytes (avx2) but they become unordered with 16 (avx=0 or avx1) or 64 (avx512) bytes vectors.

The mystery of why the test did not fail in our teting environment is also solved! We have old Skylake machines (even my local machine) for which AVX is switched to avx2:
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/vm_version_x86.cpp#L721

I have simple fix (use original loads ordering indexes) but looking on the code which causing the issue I see that it is bogus/incomplete - it does not help cases listed for JDK-8076284 changes:

https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2015-April/017645.html

Using unrolling and cloning information to vectorize is interesting idea but as I see it is not complete. Even if pack_parallel() method is able created packs they are currently all removed by filter_packs() method.
And additionally the above cases from JDK-8076284 are vectorized without hoisting loads and pack_parallel - I verified it.

The code added by JDK-8076284 is useless now and I am excluding ti. It needs more work to be useful. I reluctant to remove the code because may be in a future we will have time to invest into it.

There were 2 way to exclude it: add new field in superword class:

   bool           _do_vector_loop;  // whether to do vectorization/simd style
+  bool           _do_vector_loop_experimental; // experimental optimization
   bool           _do_reserve_copy; // do reserve copy of the graph(loop) before final modification in output

or use #if VECTOR_LOOP_SIMD as in current changes. I prefer this one to avoid wasting compilation time. I used first one for testing by setting _do_vector_loop_experimental(UseNewCode).

Testing: tier1-7 with avx2 and avx512.
Performance testing - no regrression.
I also compared jtreg tests output with -XX:+TraceNewVectors to verify that number of created vector nodes did not change.


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 running) ✔️ (9/9 passed)

Issue

  • JDK-8251994: VM crashed running TestComplexAddrExpr.java test with -XX:UseAVX=X

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 26, 2020

👋 Welcome back kvn! 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 openjdk bot added the rfr Pull request is ready for review label Oct 26, 2020
@openjdk
Copy link

openjdk bot commented Oct 26, 2020

@vnkozlov 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.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 26, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 26, 2020

Webrevs

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by comment:

  • synopsis should be "crashed", not "crushed"?
  • I personally find VECTOR_LOOP_SIMD more fragile than the _do_vector_loop_experimental field. At least the macro should also say "experimental"?

src/hotspot/share/opto/superword.cpp Show resolved Hide resolved
@vnkozlov vnkozlov changed the title 8251994: VM crushed running TestComplexAddrExpr.java test with -XX:UseAVX=X 8251994: VM crashed running TestComplexAddrExpr.java test with -XX:UseAVX=X Oct 26, 2020
@vnkozlov
Copy link
Contributor Author

Drive-by comment:

  • synopsis should be "crashed", not "crushed"?

Fixed.

  • I personally find VECTOR_LOOP_SIMD more fragile than the _do_vector_loop_experimental field. At least the macro should also say "experimental"?

'fragile' is in naming sense to use DO_VECTOR_LOOP_EXPERIMENTAL, for example?
Or you prefer to have the code be guarded by if(_do_vector_loop_experimental) runtime check instead of #if DO_VECTOR_LOOP_EXPERIMENTAL macro check?

@shipilev
Copy link
Member

'fragile' is in naming sense to use DO_VECTOR_LOOP_EXPERIMENTAL, for example?

This makes most sense, I believe.

Or you prefer to have the code be guarded by if(_do_vector_loop_experimental) runtime check instead of #if DO_VECTOR_LOOP_EXPERIMENTAL macro check?

Yeah, I am not a big fan of doing macros when runtime checks carry the same weight. I understand you want to micro-optimize these paths with macros, but I think the runtime checks work without much penalty here? I would defer to compiler reviewers to say which one is better.

@cl4es
Copy link
Member

cl4es commented Oct 26, 2020

Wouldn't something like a const bool _do_vector_loop_experimental = false; eliminate the disabled code anyway?

@vnkozlov
Copy link
Contributor Author

Wouldn't something like a const bool _do_vector_loop_experimental = false; eliminate the disabled code anyway?

Good idea. I assume you mean static const bool. I will update changes.

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is much cleaner, thanks. Looks good to me.

@openjdk
Copy link

openjdk bot commented Oct 26, 2020

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

8251994: VM crashed running TestComplexAddrExpr.java test with -XX:UseAVX=X

Reviewed-by: shade, redestad

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

  • b498433: 8254825: Monitoring available clipboard formats should be done via new Windows APIs
  • de05b00: 8255365: Problem list failing client manual tests
  • 49c4978: 8060202: [macosx] Test closed/java/awt/Choice/GetSizeTest/GetSizeTest fails only in MacOSX(10.10)
  • 2b47a58: 8028281: [TEST_BUG] [macosx] javax/swing/JTabbedPane/7024235/Test7024235.java fails
  • 83a91bf: 8253734: C2: Optimize Move nodes
  • 6666dcb: 8237363: Remove automatic is in heap verification in OopIterateClosure
  • fa64477: 8255301: Common and strengthen the code in ciMemberName and ciMethodHandle
  • 9b5a2a6: 8255349: Vector API issues on Big Endian
  • e8b75b1: 8255393: sun/security/util/DerValue/Indefinite.java fails with ---illegal-access=deny
  • 7cafe35: 8255352: Archive important test outputs in submit workflow
  • ... and 11 more: https://git.openjdk.java.net/jdk/compare/3f6abd220fa9d39bd5bd60bee3827a4680437cc6...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 openjdk bot added the ready Pull request is ready to be integrated label Oct 26, 2020
@vnkozlov
Copy link
Contributor Author

Thank you Aleksey and Claes for reviews.

@vnkozlov
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Oct 26, 2020
@openjdk 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 26, 2020
@openjdk
Copy link

openjdk bot commented Oct 26, 2020

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

  • b498433: 8254825: Monitoring available clipboard formats should be done via new Windows APIs
  • de05b00: 8255365: Problem list failing client manual tests
  • 49c4978: 8060202: [macosx] Test closed/java/awt/Choice/GetSizeTest/GetSizeTest fails only in MacOSX(10.10)
  • 2b47a58: 8028281: [TEST_BUG] [macosx] javax/swing/JTabbedPane/7024235/Test7024235.java fails
  • 83a91bf: 8253734: C2: Optimize Move nodes
  • 6666dcb: 8237363: Remove automatic is in heap verification in OopIterateClosure
  • fa64477: 8255301: Common and strengthen the code in ciMemberName and ciMethodHandle
  • 9b5a2a6: 8255349: Vector API issues on Big Endian
  • e8b75b1: 8255393: sun/security/util/DerValue/Indefinite.java fails with ---illegal-access=deny
  • 7cafe35: 8255352: Archive important test outputs in submit workflow
  • ... and 11 more: https://git.openjdk.java.net/jdk/compare/3f6abd220fa9d39bd5bd60bee3827a4680437cc6...master

Your commit was automatically rebased without conflicts.

Pushed as commit a7fa1b7.

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

@vnkozlov vnkozlov deleted the JDK-8251994 branch October 26, 2020 20:47
@@ -91,6 +91,8 @@ SuperWord::SuperWord(PhaseIdealLoop* phase) :
#endif
}

static const bool _do_vector_loop_experimental = false; // Experimental vectorization which uses data from loop unrolling.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For such purposes, I find debug VM flags a better option: they are constant in product binaries, easy to experiment/change from command-line with debug binaries, and enumerated in a single place.

Any reason to prefer static const declarations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding any flag is exception - we should avoid it if it is not needed immediately. We have a lot of flags already.
As I said currently this code is broken - I want to remove it from debug builds too.
And it is easy to modify this value later - make it a field with the same name and initialize with diagnostic UseNewCode flag value as I did for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants