Skip to content
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

Expose the ConvertToIntegerNative APIs #100993

Merged
merged 6 commits into from
Apr 24, 2024

Conversation

tannergooding
Copy link
Member

This resolves #61885

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, 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.

@tannergooding
Copy link
Member Author

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
Copy link
Member Author

Ping @dotnet/jit-contrib

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Apr 18, 2024
@JulieLeeMSFT
Copy link
Member

@BruceForstall, @anthonycanino, PTAL.

@tannergooding
Copy link
Member Author

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.

Copy link
Contributor

@DeepakRajendrakumaran DeepakRajendrakumaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@kunalspathak kunalspathak left a 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));
Copy link
Member

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 like ConvertToInt64 that returns VectorT, although not sure why its entry is not in namedintrinsiclist.h. Can you please explain?

Copy link
Member Author

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_MATH
  • NI_HW_INTRINSIC
  • NI_SIMD_AS_HWINTRINSIC
  • NI_SRCS_UNSAFE
  • NI_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);
Copy link
Member

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 for varTypeIsSmall(tgtType)?

Copy link
Member Author

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 (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.

@tannergooding
Copy link
Member Author

Rerunning CI one more time before merging. Not expecting any changes

@carlossanlop
Copy link
Member

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.

cc @JulieLeeMSFT

@carlossanlop carlossanlop merged commit f55c5a8 into dotnet:main Apr 24, 2024
177 of 187 checks passed
@tannergooding tannergooding deleted the convert-unsafe branch April 24, 2024 01:17
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* 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
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* 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
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the runtime to have deterministic floating-point to integer conversions
5 participants