-
Notifications
You must be signed in to change notification settings - Fork 4k
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
lifted comparison codegen can be improved #17261
Comments
@VSadov Can you please spell out the issue (what kind of source, what IL do we generate now, and what IL would be better)? |
added examples to the bug |
In case no newcomer wants this one, I can take care |
This seems interesting to me. Though it seems like it could lead to worse performance in some cases. For example. if i have: SomeLargeStruct? s1, s2;
//...
var b = s1 == s1; Even if these have 'null' as their value, we'll incur the costly cost of doing the == check for the underlying values. This is also surprising to me as i would not have expected the underlying == to be called if either value was null. -- That said, if we think the null case is the rare case, then maybe it's better to be branchless and jsut do the work then try to bail out early if either value is null. |
Hrmm. i don't repro this locally. The IL i see generated seems to be of the form:
Or is this only for the case of things like known primitive types? If so, then i can see how a branchless form could def make sense. |
only for primitives |
Gotcha. Then that makes a lot of sense! |
How can I see the generated IL for this? I mean, where is it being generated? |
Thanks @Joe4evr I'm getting this for built-in types:
I wasn't able to reproduce any of your cases tho. If you could create an example in sharplab.io so we see what's the generated IL and how can be optimized would be great. |
okay, this makes a lot of sense! thanks for the explanation |
@alrz I still see |
My vision loss. |
To be clear, the final codegen would look like this: // We rewrite x == y as
//
// tempx = x;
// tempy = y;
// result = (tempx.GetValueOrDefault() == tempy.GetValueOrDefault()) &
// (tempx.HasValue == tempy.HasValue);
//
// and x != y as
//
// tempx = x;
// tempy = y;
// result = (tempx.GetValueOrDefault() != tempy.GetValueOrDefault()) ||
// (tempx.HasValue != tempy.HasValue);
//
// Otherwise, we rewrite x OP y as
//
// tempx = x;
// tempy = y;
// result = (tempx.GetValueOrDefault() OP tempy.GetValueOrDefault()) &&
// (tempx.HasValue & tempy.HasValue);
// @VSadov can you confirm? |
why |
@alrz @isc30 - yes like that, and yes with regular It also makes sense to use DeMorgan dual of the
|
In the codegen of lifted comparison operators some conditional branches seem unnecessary.
They only serve to shortcircuit trivial bool operators over locals - that does not seem very profitable.
We should do something like in: dotnet/corefx#15941
( Also check dotnet/corefx#17535 if that is better )
Example:
We emit lifted equality for
left
andright
as:A more efficient would be:
The text was updated successfully, but these errors were encountered: