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

Updating Vector<T> to support nint and nuint #50832

Merged
merged 19 commits into from
Apr 20, 2021

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Apr 7, 2021

This resolves #36160

In order for this to work, GenTreeJitIntrinsic was updated to track the baseType as a CorInfoType rather than as a var_types (as discussed in #36160 (comment)). There is additionally a new JitType2PreciseVarType method which ensures that a CorInfoType is converted to a var_types in a way that preserves the sign (JITtype2varType will convert CORINFO_TYPE_UINT to CORINFO_TYPE_INT otherwise).

I ran jit-diff --pmi for --frameworks, --benchmarks, and --tests after the second commit to ensure there were no diffs due to the above changes. I then added the three latter commits supporting Vector<nint> and Vector<nuint>.

Edit: The vast majority of the PR is hardware intrinsic tests being added via the NotSupported template (so they are auto-generated, like most of the hwintrinsic tests).
Review of the PR can be simplified by just reviewing one of the generated tests since they all follow the same pattern.

Edit: This work was completed:
I am marking this as a draft until I can get some more tests up, including negative validating for nint/nuint on Vector64/128/256<T>.
I will also take a look at updating the couple places where we still have using nuint_t = System.UInt64, such as in Utf16Utility.Validation.cs

@dotnet-issue-labeler
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, 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.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 7, 2021
@tannergooding tannergooding force-pushed the intrinsic-type branch 3 times, most recently from b686f43 to f3be3d6 Compare April 7, 2021 22:12
@tannergooding tannergooding force-pushed the intrinsic-type branch 2 times, most recently from 3053f08 to 755f6e0 Compare April 8, 2021 02:30
@tannergooding
Copy link
Member Author

tannergooding commented Apr 8, 2021

@fanyang-mono, could you assist with the mono failures here?

It looks like there are two kinds of failures:

  1. Mono isn't correctly treating Vector128<nint>.Zero as unsupported
  2. Mono is faulting in src/mono/mono/mini/simd-intrinsics.c due to an assertion that shouldn't be reached (looks like its not handling the MONO_TYPE_ for nint and nuint).

Related, it looks like Mono has some hard-coded logic around ThrowForUnsupportedVectorBaseType (in src/mono/mono/mini/intrinsics.c) and that is being renamed/split into two methods for this PR.

The latter two issues look fairly straightforward to resolve, I'm just not familiar with what the correct MONO_TYPE_ is for nint or nuint.
The first issue I'm not sure where to start looking.

@fanyang-mono
Copy link
Member

@fanyang-mono, could you assist with the mono failures here?

It looks like there are two kinds of failures:

  1. Mono isn't correctly treating Vector128<nint>.Zero as unsupported
  2. Mono is faulting in src/mono/mono/mini/simd-intrinsics.c due to an assertion that shouldn't be reached (looks like its not handling the MONO_TYPE_ for nint and nuint).

Related, it looks like Mono has some hard-coded logic around ThrowForUnsupportedVectorBaseType (in src/mono/mono/mini/intrinsics.c) and that is being renamed/split into two methods for this PR.

The latter two issues look fairly straightforward to resolve, I'm just not familiar with what the correct MONO_TYPE_ is for nint or nuint.
The first issue I'm not sure where to start looking.

nint and nuint is currently not support by mono. @vargaz Zoltan, could you please provide a little more details, if there is any?

@tannergooding
Copy link
Member Author

nint and nuint is currently not support by mono

I'm not sure I understand this, they are just aliases for System.IntPtr and System.UIntPtr as far as the JIT type system is concerned?

@vargaz
Copy link
Contributor

vargaz commented Apr 8, 2021

IntPtr/UIntPtr is MONO_TYPE_I/MONO_TYPE_U.

@fanyang-mono
Copy link
Member

nint and nuint is currently not support by mono

I'm not sure I understand this, they are just aliases for System.IntPtr and System.UIntPtr as far as the JIT type system is concerned?

If that's the case, they should be mapped to MONO_TYPE_I and MONO_TYPE_U. Sorry that I misunderstood them with struct types.

@tannergooding
Copy link
Member Author

tannergooding commented Apr 8, 2021

I believe I've fixed the first issue (70432e5):

Mono isn't correctly treating Vector128.Zero as unsupported

and the third issue (c3107e0):

it looks like Mono has some hard-coded logic around ThrowForUnsupportedVectorBaseType (in src/mono/mono/mini/intrinsics.c) and that is being renamed/split into two methods for this PR.

I've also fixed part of the second issue (also c3107e0):

Mono is faulting in src/mono/mono/mini/simd-intrinsics.c due to an assertion that shouldn't be reached (looks like its not handling the MONO_TYPE_ for nint and nuint).

The remainder of the second issue looks to be that a couple code paths will still assert (type_to_expand_op, type_to_insert_op, and type_to_extract_var_op) since MONO_TYPE_I8 and MONO_TYPE_U8 aren't handled.
It isn't clear if this is better handled by picking an existing op (e.g. OP_EXPAND_I4 when targeting 32-bit and OP_EXPAND_I8 when targeting 64-bit) or if it is better to add a new op (e.g. OP_EXPAND_I) and plumb the handling through.

@fanyang-mono
Copy link
Member

I believe I've fixed the first issue (70432e5):

Mono isn't correctly treating Vector128.Zero as unsupported

and the third issue (c3107e0):

it looks like Mono has some hard-coded logic around ThrowForUnsupportedVectorBaseType (in src/mono/mono/mini/intrinsics.c) and that is being renamed/split into two methods for this PR.

I've also fixed part of the second issue (also c3107e0):

Mono is faulting in src/mono/mono/mini/simd-intrinsics.c due to an assertion that shouldn't be reached (looks like its not handling the MONO_TYPE_ for nint and nuint).

The remainder of the second issue looks to be that a couple code paths will still assert (type_to_expand_op, type_to_insert_op, and type_to_extract_var_op) since MONO_TYPE_I8 and MONO_TYPE_U8 aren't handled.
It isn't clear if this is better handled by picking an existing op (e.g. OP_EXPAND_I4 when targeting 32-bit and OP_EXPAND_I8 when targeting 64-bit) or if it is better to add a new op (e.g. OP_EXPAND_I) and plumb the handling through.

Maybe something like this

case MONO_TYPE_I:
#if TARGET_SIZEOF_VOID_P == 8
<treat it the same as I8>
#else
<treat it as I4>
#endif

@tannergooding
Copy link
Member Author

tannergooding commented Apr 8, 2021

I believe the relevant mono paths are now updated to correctly handle Vector<nint> and Vector<nuint> and to block the same for Vector64/128/256<T> (at least until we take those to API review and hopefully unblock them as well). Hopefully CI passes now, thanks for the help! 😄

@tannergooding tannergooding marked this pull request as ready for review April 8, 2021 21:44
@tannergooding
Copy link
Member Author

CC. @echesakovMSFT, @dotnet/jit-contrib for review

@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 8, 2021
@lambdageek
Copy link
Member

@vargaz could you review the mono commit 8eb6fe9

@vargaz
Copy link
Contributor

vargaz commented Apr 9, 2021

The mono changes look ok to me.

@tannergooding
Copy link
Member Author

Rebased to resolve a conflict, believe I've also resolved the remaining ARM64 issue (it passed on my Windows box).

@tannergooding
Copy link
Member Author

The mono changes look ok to me.

@vargaz, I had to push another commit for Mono: 0f4764d

A bit of the handling existed in mini-amd64.c and so was missed in my first pass, but caught by the tests.
I think I've gotten it all this time, but we'll see what CI says 😄

@tannergooding
Copy link
Member Author

Everything is passing now except for two LLVM AOT scenarios. It isn't clear to me what's wrong here given that the regular Mono leg passed.

@vargaz or @fanyang-mono, could you provide some guidance on how to root cause the failure here?

@tannergooding
Copy link
Member Author

Actually, I may have found it, there is a single line calling out an assert about halfway through the log:

  • Assertion: should not be reached at /__w/1/s/src/mono/mono/mini/mini-llvm.c:580

@tannergooding
Copy link
Member Author

Only test failures are unrelated and due to #51346

This should be ready for final review/merge.

@GrabYourPitchforks
Copy link
Member

My small contribution to the review: commit 57fc73d LGTM!

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you for your contribution, @tannergooding!

cc @dotnet/jit-contrib

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider expanding Vector<T> to support nint and nuint
7 participants