Expose the ConvertToIntegerNative APIs#100993
Conversation
ghost
added
|
Note regarding the |
tannergooding
force-pushed
the
convert-unsafe
branch
2 times, most recently
from
aea06be to
160402e
Compare
tannergooding
force-pushed
the
convert-unsafe
branch
2 times, most recently
from
6a06e7b to
272b8f6
Compare
tannergooding
force-pushed
the
convert-unsafe
branch
from
272b8f6 to
5e02846
Compare
|
CC. @dotnet/jit-contrib, @dotnet/avx512-contrib This exposes the APIs that allow people to opt-in to the platform specific floating-point conversion behavior. It is a continuation of #97529 |
|
Ping @dotnet/jit-contrib |
|
@BruceForstall, @anthonycanino, PTAL. |
|
It'd be nice to get this in before the snap tomorrow, so that we have a complete experience for the standardization around floating-point conversions that was done and users have the escape hatch to get access to the prior behavior. |
kunalspathak
left a comment
kunalspathak
left a comment
There was a problem hiding this comment.
JIT changes LGTM, with some questions.
|
|
||
| if (!varTypeIsArithmetic(retType)) | ||
| { | ||
| assert((intrinsic == NI_PRIMITIVE_ConvertToInteger) || (intrinsic == NI_PRIMITIVE_ConvertToIntegerNative)); |
There was a problem hiding this comment.
I am not too familiar with "named intrinsics", but aren't there more intrinsics like ConvertToInt64 that returns VectorT, although not sure why its entry is not in namedintrinsiclist.h. Can you please explain?
There was a problem hiding this comment.
They are, they are just included indirectly via the tables. That's what this region is for: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/namedintrinsiclist.h#L140-L164
We basically just have 6 categories within the list:
NI_SYSTEM_MATHNI_HW_INTRINSICNI_SIMD_AS_HWINTRINSICNI_SRCS_UNSAFENI_PRIMITIVE- general intrinsics not in another list
Each of the first 5 categories are just groups of intrinsics that generally get handled together via some central function for that group.
So the ConvertToInt64 for Vector<T> is handled by NI_SIMD_AS_HWINTRINSIC and won't hit this path.
|
|
||
| var_types tgtType = JitType2PreciseVarType(sig->retType); | ||
| retType = genActualType(retType); | ||
| bool uns = varTypeIsUnsigned(tgtType) && !varTypeIsSmall(tgtType); |
There was a problem hiding this comment.
why we do not set this for varTypeIsSmall(tgtType)?
There was a problem hiding this comment.
That's just how IL importation works for conversions in general (see CEE_CONV in importer.cpp) and we're matching that.
I imagine it has to do with the fact that the "evaluation stack" type is never a small type.
|
Rerunning CI one more time before merging. Not expecting any changes |
|
We need this change included in the Preview4 snap and the merge-on-green restriction is blocking us because a couple of failures are not getting linked to KnownBuildError issues as expected. @tannergooding has confirmed the failures are all unrelated, so I will bypass the requirements and merge it. |
* Expose the ConvertToIntegerNative APIs for the floating-point types * Accelerate the ConvertToInteger and related APIs * Applying formatting patch * Fixing some tests for x86 and skipping some tests on Mono
* Expose the ConvertToIntegerNative APIs for the floating-point types * Accelerate the ConvertToInteger and related APIs * Applying formatting patch * Fixing some tests for x86 and skipping some tests on Mono
5 participants
This resolves #61885