Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unify Default Comparer logic #88006
Unify Default Comparer logic #88006
Changes from 44 commits
7f617e2
2536992
5605e0e
72e548a
9d9fc84
f229039
70cb1c0
e095936
d591efe
7088336
0c0c84d
c516e7f
d8195e6
f8249c7
4d1d88f
5ab501a
6c45b7e
40eab70
ff59591
1f2b4aa
d0627bf
ba2e5cb
6bdbee2
1d14835
f4009d7
6fdb5c9
b085174
7659053
8ec024f
f5a9c18
c33e1de
588d884
a64df29
fa682a2
859a25d
45ec76d
1c79052
d0ad2dd
8471b21
1999819
0772c85
35ee0a2
7c902b4
fc21b4f
fff6180
1b1c1e7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
CanCastTo(IEquatable)
case needs to be after this check, like I have suggested in my comment.Or the
IsCanonicalSubtype
check needs to be the very first thing in the method - we would give up on more cases that way. (It would mirror what happens today.)Or the
CanCastTo(IEquatable)
check needs to be implemented against generic type definition for theIsCanonicalSubtype
case. This would be the most precise option, but there would be still some ambiguous cases where we would have to give up.Here is example of a bug that would be hit by the code as written:
It will start printing true correctly, and then flips to printing false as the incorrect Tier1 optimization kicks in.
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.
It wouldn't, it'd then block
Struct<_Canon>
too, which you were concerned about before.The issue right now is that this seems to still be devirtualizing
__Canon
as ObjectEqualityComparer for some reason (see those diffs). I have no idea what's going on here, could WellKnownArg be returning some weird handle 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.
Right, it won't mirror the bugs. Notice that the bug demonstrated by my
G<T,U>
repro has been shipping for a while (just tried it on .NET 7).Struct<_Canon>
needs the third option (use generic type definition to check for IEquatable implementation). Otherwise, it is better to block it rather than to have functional bugs that manifest as observable behavior differences between Tier0 and Tier1 JIT.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 links to my comment. Did you meant to link to some diffs somewhere?
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.
Yeah, seems like
Copy link
on that GitHub message didn't work so I still had a link to your comment in my clipboard instead. Fixed that link.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.
There diffs look odd. Are you able to reproduce the behavior locally? It can be that the bot picked up stale bits from somewhere.
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 new diffs look good to me, but I do not think the difference can be explained by the one commit that you have pushed.
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 bot seems to be having some issues with picking up VM changes, after a fix CoreLib changes look good but frameworks ones still have the weird diff, not sure if it's still an issue with the bot.