-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Centralize the folding logic for ConditionalSelect and ensure side effects aren't dropped #104175
Conversation
…fects aren't dropped
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
e2bc3d1
to
e77d682
Compare
e77d682
to
554e3cc
Compare
CC. @dotnet/jit-contrib, this should be ready for review. There's some decent diffs:
Where most of this comes from us properly recognizing - vpmovm2d zmm1, k1
- vpternlogd zmm0, zmm0, zmm1, 85
- vpmovd2m k1, zmm0
+ knotw k1, k1 There's, however, a couple small regressions where the - vpcmpeqd xmm2, xmm2, xmm2
- vpternlogd xmm1, xmm0, xmm2, 86
- vptest xmm1, xmm1
+ vpor xmm0, xmm1, xmm0
+ vpcmpeqd xmm1, xmm1, xmm1
+ vpxor xmm0, xmm1, xmm0
+ vptest xmm0, xmm0 I want to handle these separately where I can also fixup the logic to do proper containment support for this case and get the rest of the wins possible here. |
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. Added few questions.
break; | ||
} | ||
|
||
if (nestedIsScalar) |
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.
This is always false
so we do not need it or did you forget to set it?
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.
This exists as a form of future proofing, its expected that isScalar could be true and we don’t handle that case here
/* | ||
/- A | ||
+- B | ||
t1 = binary logical op1 |
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.
Just to make sure I follow the example in the comment - It is trying to find op1 and op2 has nested opertions basically.
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.
This is finding a bitwise operation that contains a nested bitwise operation so they can become a single ternary operation
bool nestedIsScalar = false; | ||
genTreeOps nestedOper = second->AsHWIntrinsic()->HWOperGet(&isScalar); | ||
|
||
if (nestedOper == GT_NONE) |
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.
don't think we need this condition. It will be taken care by the condition on line 1468 below.
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.
The one further down is temporary and will be removed so we can handle other cases such as not, and_not, and other other future binary operations
This resolves #104116