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

Test result of arraycmp for value type comparison #16776

Merged

Conversation

hzongaro
Copy link
Member

The arraycmp operation produces a result of zero if the memory being compared is equal, one if the first operand is less than the second, or two if the second operand is less than the first. The transformation of <object{Equality|Inequality}Comparison> used arraycmp assuming that the result would be zero for equality or one for inequality. This change adds an icmpeq or icmpne to compare the result of the arraycmp with zero in order to yield a value in the expected range: {0,1}.

@hzongaro
Copy link
Member Author

Leaving this as a draft pull request while running tests.

@hzongaro hzongaro marked this pull request as ready for review February 27, 2023 20:28
@hzongaro
Copy link
Member Author

Annabelle @a7ehuo, Daryl @0xdaryl, may I ask you to review this change?

Copy link
Contributor

@a7ehuo a7ehuo left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for fixing this bug! Just minor comments on the code comments

runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved
@hzongaro
Copy link
Member Author

Thanks, Annabelle @a7ehuo! I have adjusted the comments in commit 5555dc7cdbe1aef429959c6471286298fc4efd4e. If you are OK with those changes, I will squash the commits.

@a7ehuo
Copy link
Contributor

a7ehuo commented Feb 28, 2023

If you are OK with those changes, I will squash the commits.

LGTM. I've approved the change

The arraycmp operation produces a result of zero if the memory being
compared is equal, one if the first operand is less than second, or two
if the second operand is less than the first.  The transformation of
<object{Equality|Inequality}Comparison> used arraycmp assuming that the
result would be zero for equality or one for inequality.  This change
adds an icmpeq or icmpne to compare the result of the arraycmp with zero
in order to yield a value in the expected range:  {0,1}.

Signed-off-by:  Henry Zongaro <zongaro@ca.ibm.com>
@hzongaro hzongaro force-pushed the test-arraycmp-result-for-value-type branch from 5555dc7 to 1bbd087 Compare March 1, 2023 15:00
@hzongaro
Copy link
Member Author

hzongaro commented Mar 1, 2023

Squashed these changes into commit 1bbd087

@hzongaro
Copy link
Member Author

hzongaro commented Mar 1, 2023

Jenkins test sanity xlinuxval jdknext

@hzongaro
Copy link
Member Author

hzongaro commented Mar 1, 2023

Jenkins test sanity all jdk17

@hzongaro hzongaro added comp:jit project:valhalla Used to track Project Valhalla related work labels Mar 2, 2023
@hzongaro hzongaro added this to TODO: VM in Valhalla L-World via automation Mar 2, 2023
@hzongaro
Copy link
Member Author

@0xdaryl Daryl, have you had a chance to look at this change?

@0xdaryl 0xdaryl self-assigned this Mar 31, 2023
Valhalla L-World automation moved this from TODO: VM to Reviewer approved Mar 31, 2023
@0xdaryl 0xdaryl merged commit a795997 into eclipse-openj9:master Mar 31, 2023
Valhalla L-World automation moved this from Reviewer approved to Done Mar 31, 2023
@hzongaro hzongaro deleted the test-arraycmp-result-for-value-type branch April 5, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit project:valhalla Used to track Project Valhalla related work
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants