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

Experiment with compiler support for nint and nuint **NOMERGE** #36158

Closed

Conversation

GrabYourPitchforks
Copy link
Member

We now have a compiler toolchain that supports nint and nuint natively.

The remaining nint / nuint usages in Span<T> and ReadOnlySpan<T> are using the compiler shortcuts for IntPtr and UIntPtr. I created some experimental overloads of APIs in Buffer and Unsafe to act as the targets.

This is not intended to represent production code and will not be committed.
It is meant to exercise CI code paths to see if anything falls over unexpectedly on other architectures.

@GrabYourPitchforks GrabYourPitchforks added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 9, 2020
@ghost
Copy link

ghost commented May 9, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented May 9, 2020

Did you run into any gotchas with deleting aliases for nuint/uint from everywhere and then just fix the compile errors? I would expect it to be pretty straightforward.

@@ -10,11 +10,11 @@

#pragma warning disable SA1121 // explicitly using type aliases instead of built-in types
#if TARGET_64BIT
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering why this isn't testing things by just removing these aliases? I'd expect us to be able to just remove all of the usings and have things "just work"
If they don't, its likely a compiler bug we should flag @cston on and otherwise, we should be generally good to check things in.
The compiler just uses the built-in IL operators after all and we were already using aliases of the right size....

Copy link
Member

@jkotas jkotas May 9, 2020

Choose a reason for hiding this comment

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

I gave it a shot: #36159 . Some places needed an extra unsafe or cast, but it was pretty straightforward otherwise.

@GrabYourPitchforks
Copy link
Member Author

I didn't remove the aliases straight-up because I anticipated there might be problems at [Intrinsic] methods since those might require VM changes as well. Wanted to minimize the number of potential errors for the first attempt.

@jkotas
Copy link
Member

jkotas commented May 9, 2020

Intrinsic

There should not be any (for CoreCLR at least). https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/mscorlib.h does not have any 64-bit ifdefs.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants