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
Adding System.Runtime.CompilerServices.Unsafe.BitCast #82917
Adding System.Runtime.CompilerServices.Unsafe.BitCast #82917
Changes from 11 commits
606fa6b
9788980
a8bd8a6
38e4b87
5cdf971
df296f4
aae3cfd
e7cd147
e2875dd
b342756
bc80205
29afa95
22012d8
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.
can this whole condition be simplified to
?
(nit)
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 that code is much less obvious and harder to read/understand. It's the kind of thing that you have to stop and think about boolean tables to remember what it does.
The current code is a bit more verbose, but I think that almost anyone can understand what's happening.
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.
can we move these foldings to
gtFoldExprConst
? (or I'd expect them to be already there) so we can just emit bitcast here and have much simpler logic - it is a bit hard to read. Although, you don't even need to callgtFoldExprConst
here, it will likely be called by someone else in importer, e.g. if it's part of a condition for a branchThere 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.
We can, but I think it can/should be a separate PR given it has to touch a bit of unrelated other code that exists.
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.
Notably, we can't "easily" do this since it would regress handling on 32-bit for double/int64. Once we add the relevant decomposition support then it won't be so problematic.
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.
Should this check
IsReferenceOrContainsReferences
onTFrom
/TTo
and throw?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.
Simply checking that
IsReferenceOrContainsReferences
is the same betweenTFrom
andTTo
will only catch a subset of cases. It won't catch the more complex cases where they match, but the layouts are then considered "incompatible".The implementation will "do the right thing" as the software fallback just uses
As<TFrom, TTo>(ref value)
and the intrinsic logic checks for compatible layouts. It's just that them being actually incompatible may result in UB, much as it would forAs
or many of the otherUnsafe
APIs.