Adding System.Runtime.CompilerServices.Unsafe.BitCast#82917
Adding System.Runtime.CompilerServices.Unsafe.BitCast#82917tannergooding merged 13 commits intodotnet:mainfrom
Conversation
ghost
added
area-CodeGen-coreclr
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis resolves #81334
|
|
Where should we be using this in runtime? |
Working on getting that up. We can use it in |
|
Does this also need adding to mono? |
Yesn't. Since there is a software fallback, Mono isn't going to fail. MonoLLVM looks to already be doing the right things but might benefit throughput wise if it was special cased. The same goes for MonoJIT and WASM. |
|
CC. @dotnet/jit-contrib |
|
|
||
| bool involvesStructType = false; | ||
|
|
||
| if (fromType == TYP_STRUCT) |
There was a problem hiding this comment.
can this whole condition be simplified to
if ((fromType == TYP_STRUCT) ^ ((fromType == TYP_STRUCT))
return null;
if ((fromType == TYP_STRUCT) && ((fromType == TYP_STRUCT))
...
?
(nit)
There was a problem hiding this comment.
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.
|
|
||
| GenTree* op1 = impPopStack().val; | ||
|
|
||
| if (op1->IsCnsFltOrDbl()) |
There was a problem hiding this comment.
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 call gtFoldExprConst here, it will likely be called by someone else in importer, e.g. if it's part of a condition for a branch
There was a problem hiding this comment.
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.
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.
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/Unsafe.cs
Outdated
Show resolved
Hide resolved
| public static TTo BitCast<TFrom, TTo>(TFrom source) | ||
| where TFrom : struct | ||
| where TTo : struct | ||
| { |
There was a problem hiding this comment.
Should this check IsReferenceOrContainsReferences on TFrom/TTo and throw?
There was a problem hiding this comment.
Simply checking that IsReferenceOrContainsReferences is the same between TFrom and TTo 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 for As or many of the other Unsafe APIs.
7 participants
This resolves #81334