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

Fix b2iEvaluator in power for bRegLoad #5748

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

mnalam-p
Copy link
Contributor

@mnalam-p mnalam-p commented Jan 13, 2021

The b2iEvaluator currently does not generate sign extension for
bRegLoad. While it will work for sign extended bRegLoad, it may
yield wrong result if the byte was not sign extended. This commit
cover such cases, by making sure bRegLoad is always sign extended.

Fixes: eclipse-openj9/openj9#11192
Signed-off-by: Mohammad Nazmul Alam [email protected]

@mnalam-p
Copy link
Contributor Author

@gita-omr kindly review the changes. Thanks in advance.

@gita-omr
Copy link
Contributor

I think the PR description needs to be changed. JProfiler just exposed the problem in the Power b2iEvaluator. That problem can be exposed in many other situations.

@gita-omr
Copy link
Contributor

I think it's better to describe the issue in the evaluator itself. It assumed that bRegload has byte sign extended and therefore skipped the extension. However the value of bRegload node is not necessarily sign extended.

The b2iEvaluator currently does not generate sign extension for
bRegLoad. While it will work for sign extended bRegLoad, it may
yield wrong result if the byte was not sign extended. This commit
cover such cases, by making sure bRegLoad is always sign extended.

Fixes: eclipse-openj9/openj9#11192
Signed-off-by: Mohammad Nazmul Alam <[email protected]>
@mnalam-p mnalam-p force-pushed the cons_charset_failure branch from 993979e to 5ae1d2b Compare January 14, 2021 21:39
@gita-omr
Copy link
Contributor

@aviansie-ben could you please review/merge ?

Copy link
Contributor

@aviansie-ben aviansie-ben left a comment

Choose a reason for hiding this comment

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

LGTM

@aviansie-ben
Copy link
Contributor

@genie-omr build plinux,aix

@aviansie-ben aviansie-ben merged commit 1e63531 into eclipse-omr:master Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MauveMultiThreadLoadTest_special_16 String.ConsCharset got 65503 but expected 223
3 participants