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

8258134: assert(size == calc_size) failed: incorrect size calculation on x86_32 with AVX512 machines #1753

Closed
wants to merge 1 commit into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Dec 11, 2020

Hi all,

Two vector api tests crashed on x86_32 with AVX512 machines due to this assert [1].
The reason is that 'calc_size' is incorrect.

But there is no need to calculate 'calc_size' manually at all since the result [2] is actually never used by the VM.
Also, it is really hard to maintain the calculation logic for various hardwares and configurations.
And it may be easily broken again in the future with more and more complicated instructions & configurations.

So it would be better to remove the calculation and the assert, which is safe and already done for x86_64 [3].
The fix just follows what is done for x86_64.

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/x86_32.ad#L1016
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/x86_32.ad#L1059
[3] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/x86_64.ad#L1042


Progress

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

Issue

  • JDK-8258134: assert(size == calc_size) failed: incorrect size calculation on x86_32 with AVX512 machines

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 11, 2020

👋 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.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 11, 2020

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

Loading

@openjdk openjdk bot added the rfr label Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@DamonFool
The hotspot-compiler label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@DamonFool The hotspot-compiler label was already applied.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 12, 2020

Webrevs

Loading

@@ -1088,18 +1059,19 @@ uint MachSpillCopyNode::implementation( CodeBuffer *cbuf, PhaseRegAlloc *ra_, bo
// mem -> mem
int src_offset = ra_->reg2offset(src_first);
int dst_offset = ra_->reg2offset(dst_first);
return vec_stack_to_stack_helper(cbuf, do_size, src_offset, dst_offset, ireg, st);
vec_stack_to_stack_helper(cbuf, src_offset, dst_offset, ireg, st);
Copy link
Contributor

@neliasso neliasso Dec 14, 2020

Choose a reason for hiding this comment

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

None of these functions (vec_stack_to_stack_helper, vec_mov_helper, vec_spill_helper, vec_spill_helper) returns a value that is used on either x86 or x64 now.

Please make them all return void.

Loading

Copy link
Member Author

@DamonFool DamonFool Dec 15, 2020

Choose a reason for hiding this comment

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

Thanks @neliasso for your review.
Will do it later.

And I'll close this pr and create a new one into jdk16 since this issue had been targeted to jdk16.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 15, 2020

The new pr is here: openjdk/jdk16#21
Thanks.

Loading

@DamonFool DamonFool closed this Dec 15, 2020
@DamonFool DamonFool deleted the JDK-8258134 branch Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants