- Notifications
You must be signed in to change notification settings - Fork5.2k
Expose theConvertToIntegerNative APIs#100993
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedApr 12, 2024
Note regarding the |
aea06be to160402eCompare6a06e7b to272b8f6Comparetannergooding commentedApr 13, 2024
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 |
tannergooding commentedApr 18, 2024
Ping @dotnet/jit-contrib |
JulieLeeMSFT commentedApr 18, 2024
@BruceForstall,@anthonycanino, PTAL. |
tannergooding commentedApr 22, 2024
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. |
DeepakRajendrakumaran left a comment
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.
LGTM
kunalspathak left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not too familiar with "named intrinsics", but aren't there more intrinsics likeConvertToInt64 that returns VectorT, although not sure why its entry is not innamedintrinsiclist.h. Can you please explain?
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.
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 theConvertToInt64 forVector<T> is handled byNI_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why we do not set this forvarTypeIsSmall(tgtType)?
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.
That's just how IL importation works for conversions in general (seeCEE_CONV inimporter.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.
tannergooding commentedApr 23, 2024
Rerunning CI one more time before merging. Not expecting any changes |
carlossanlop commentedApr 24, 2024
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
Thisresolves#61885