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.

Sign up for GitHub

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

Jump to bottom

Fix encoding of smulh/umulh instructions on AArch64 #7104

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Fix encoding of smulh/umulh instructions on AArch64 #7104

merged 1 commit into from
Aug 31, 2023

Conversation

Akira1Saitoh
Copy link
Contributor

@Akira1Saitoh Akira1Saitoh commented Aug 31, 2023 •
edited
Loading

AArch64 code generator incorrectly encodes smulh and umulh instructions, and those malformed instructions produce unexpected results. This commit fixes encoding of those instructions and adds binary encoding unit tests.

Issue eclipse-openj9/openj9#18057

@Akira1Saitoh Akira1Saitoh mentioned this pull request Aug 31, 2023
Closed
@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

Jenkins build aarch64,amac

@knn-k knn-k self-assigned this Aug 31, 2023
@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

I checked the binary encodings of those instructions. The Ra field (bits 10-14) need to be all 1. The change in this PR looks OK to me.

@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

Please update the comment in OMRInstOpCode.enum, too.

@Akira1Saitoh
Copy link
Contributor Author

Updated the comment in OMRInstOpCode.enum.

@0xdaryl
Copy link
Contributor

0xdaryl commented Aug 31, 2023

Could you reference the issue (18057) in the commit/PR message for archeological purposes? Don't force the issue to be closed when this PR is merged as we will want to get this into OpenJ9 0.41 as well.

Fix encoding of smulh/umulh instructions on AArch64
0cd2556 
AArch64 code generator incorrectly encodes `smulh` and `umulh` instructions,
and those malformed instructions produce unexpected results.
This commit fixes encoding of those instructions and adds binary encoding
unit tests.

Issue eclipse-openj9/openj9#18057

Signed-off-by: Akira Saitoh <saiaki@jp.ibm.com>
@Akira1Saitoh
Copy link
Contributor Author

Updated the commit and PR message to reference the issue.

@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

Jenkins build aarch64,amac

@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

The smulh/umulh instructions were added by PR #2600 in 2018.
It seems lmulhEvaluator() has not been called on AArch64 till PR #7095 was merged last week.

@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

See Issue #6516 for the socket test failures in porttest on x86-64 macOS. It is not related to this PR.
Merging.

@knn-k knn-k merged commit 54d66f8 into eclipse:master Aug 31, 2023
6 of 8 checks passed
@knn-k
Copy link
Contributor

knn-k commented Aug 31, 2023

@Akira1Saitoh Please prepare the same fix for the v0.41 branch. Thanks.

@Akira1Saitoh Akira1Saitoh mentioned this pull request Aug 31, 2023
Merged
@Akira1Saitoh
Copy link
Contributor Author

I opened eclipse-openj9/openj9-omr#185 for v0.41.

@Akira1Saitoh Akira1Saitoh deleted the aarch64FixSmulh branch August 31, 2023 23:01
@Akira1Saitoh Akira1Saitoh mentioned this pull request Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knn-k knn-k knn-k approved these changes

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl 0xdaryl is a code owner

@charliegracie charliegracie Awaiting requested review from charliegracie

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

Assignees

@knn-k knn-k

Labels
arch:aarch64 comp:compiler comp:test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Akira1Saitoh @knn-k @0xdaryl