-
Notifications
You must be signed in to change notification settings - Fork 76
8260585: AArch64: Wrong code generated for shifting right and accumulating four unsigned short integers #136
Conversation
…ting four unsigned short integers
👋 Welcome back dongbo! A progress list of the required criteria for merging this PR into |
/label add hotspot-dev |
@dgbo |
Webrevs
|
/label add hotspot-compiler |
@vnkozlov |
Someone familiar with Aarch64 assembler have to review this before it is approved for JDK 16. |
@dgbo 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:
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 10 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the 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 (@veresov, @dean-long, @nsjian, @theRealAph) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Why didn't the testing for JDK-8255949 catch this? Do you need to fix the regression test too? |
Yes, we need regression test for this fix. Or modify existing one to catch it. |
Did not run local tests for small loops in JDK-8255949. |
Mailing list message from Andrew Haley on hotspot-dev: On 1/30/21 5:07 AM, Dong Bo wrote: I don't understand. Looking at this: instruct vsrla4S_imm(vecD dst, vecD src, immI shift) %{ What happens when the shift is >= 16? What happens to src and dst? -- |
This was wrong, both src and dst should have the same value as before.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Hi, Andrew. The reason
How do you think if we do this modification via this PR? |
I think this is an enhancement, and should be done in a separate patch in jdk mainline. |
OK, I update a test with loop size 80 for bytes so that |
Ping... Can I get a review for the newest changes? Please let me know if we are ready to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks.
Thank you all for the review. |
/sponsor |
@dean-long @dgbo Since your change was applied there have been 10 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 5307afa. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This is a typo introduced by JDK-8255949.
Compiler will generate
ushr
for shifting right and accumulating four short integers.It produces wrong results for specific case. The instruction should be
usra
.Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk16 pull/136/head:pull/136
$ git checkout pull/136