-
Notifications
You must be signed in to change notification settings - Fork 416
Combine bu2i comparison optimization in z/codegen #8050
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
Conversation
10e53e4 to
2f10d2f
Compare
|
This commit fixes eclipse-openj9/openj9#22835 @r30shah Can I get a review? |
r30shah
left a comment
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.
@dchopra001 This is the fix for the J9 issue Henry has pointed out. Please change the commit message accordingly to reflect that.
| if (root->getOpCode().isBooleanCompare() || nonClobberingDestination) { | ||
| if (firstChild->getOpCodeValue() == TR::bu2i && firstChild->getReferenceCount() == 1 | ||
| if ((firstChild->getOpCodeValue() == TR::bu2i && firstChild->getReferenceCount() == 1 | ||
| && firstChild->getRegister() == NULL |
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.
I feel that indentation does not help with the the condition here. Can you apply the recommendation from the clang format error and see how it looks ?
c8629b4 to
5610daa
Compare
joransiu
left a comment
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.
LGTM.
|
@dchopra001 I think it should be |
r30shah
left a comment
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.
@hzongaro Can I request your review / merge of this PR ?
The z/codegen skips evaluation of a comparison operation if the first OR second child is a bu2i node and its grandchild is a bloadi. This condition needs to be modified from OR to AND (i.e so that both children of the comparison operation satisfy all conditions). This fixes a bug where the second comparison child has an incompatible type with the bu2i node. Fixes: eclipse-openj9/openj9#22835 Signed-off-by: Dhruv Chopra <[email protected]>
5610daa to
c1075d5
Compare
hzongaro
left a comment
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.
I think this change looks good. I will test it out in conjunction with draft pull request #8005.
|
Sorry for the long delay. I've made a number of attempts to reproduce the failure that I had been seeing with CFG Simplifier enabled in order to verify that this change fixes the problem. I haven't managed to reproduce that failure again; however, I think your fix looks correct, so I will go ahead and approve this pull request. |
hzongaro
left a comment
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. Thanks!
|
Jenkins build zos,zlinux |
|
z/os testing timed out as nodes are off-line. zLinux testing was successful. The change is safe as it makes the optimization more conservative. Merging. |
The z/codegen skips evaluation of a comparison operation if the first OR second child is a bu2i node and its grandchild is a bloadi. This condition needs to be modified from OR to AND (i.e so that both children of the comparison operation satisfy all conditions). This fixes a bug where the second comparison child has an
incompatible type with the bu2i node.