Typeforward System.Runtime.CompilerServices.Unsafe to SPC for netcore

[marek-safar]

Right now we have two different implementations of System.Runtime.CompilerServices.Unsafe type. One in System.Runtime.CompilerServices assembly and the second one in System.Private.Corelib assembly. We should unify them and type forward to SPC implementation. This would reduce code duplication and simplify JIT/AOT maintenance for the intrinsics as well as drop dependency on IL compiler for libraries build.

[stephentoub]

To clarify, this is more than type-forwarding:

• The one in corelib is actually Internal.Runtime.CompilerServices.Unsafe
• They do not currently have the same surface area. Methods have only been added to the internal one as they're needed.

[marek-safar]

this is more than type-forwarding

Yep, that's why I wrote "We should unify them and type forward ..."

They do not currently have the same surface area.

Is there any reason why they should not have the same surface area? Does the lack of lack of C# declaration in SPC buy us anything if they are runtime intrinsics anyway?

[stephentoub]

Does the lack of lack of C# declaration in SPC buy us anything if they are runtime intrinsics anyway?

I don't understand. The ones not declared in C# also aren't implemented in the runtime.

[marek-safar]

I meant that we have 3 different types of implementations (intrinsics, SPC/C# version, SRCU/IL version). I see that SPC version is missing about 2 different method sets (e.g. Subtract* with overloads). I didn't look closely but I expect the missing methods can be implemented as C# code in SPC which would expand the non-intrinsics methods in SPC and we could type-forward. I don't know what is the history about having only a subset of them available in SPC only though and if there is an important reason for that but as of today not all Unsafe methods in SPC are implemented as intrinsics for all codegens.

[stephentoub]

I don't know what is the history about having only a subset of them available in SPC

They were just added as they were needed by corelib.

[joperezr]

@stephentoub do you think this would be feasible to do? Also @marek-safar do we know more or less how big the size wins would be here? Just trying to see if the end goal will be worth the effort here to see if we should plan this for 6.0.

[stephentoub]

do you think this would be feasible to do?

It's feasible. I don't know if it's worthwhile or not. And we would still need to maintain the ilproj implementation for the OOB.

If it moves the needle, it's "just work" (adding the missing APIs and implementations provided by the JIT, renaming things, type forwarding, whatever package work is required, etc.) It might also have an implication on being able to extend the type further in the OOB.

[marek-safar]

@joperezr I think the size win is probably around 4k but also drop of an assembly file which might be worth more than.

[joperezr]

I might be missing something, but if we are going to need the typeforward then presumably we wouldn't be getting rid of the assembly file right?

[marek-safar]

if we are going to need the typeforward then presumably we wouldn't be getting rid of the assembly file right?

The illinker by default rewrites type forwarders with the declaration types.

[tannergooding]

I probably won't be able to get to it this week. My own timeframe otherwise depends on my availability outside of the generic math work.

I'd be fine with someone else picking up the IL expansion part if they were willing/able.

[jkotas]

IL expansion

Note that the Mono side of Internal.Runtime.CompilerServices.Unsafe is implemented as JIT and interpreter intrinsics. The IL expansion won't apply there.

[Wraith2]

I have been working on the this. I've started by changing the internal Unsafe implementation and to the System namespace and fixing up corelib to refer to it that way. I've then cross referenced the CoreCLR Crossgen2 and Mono source and identifying what is implemented by each of them and it's interesting in places so I thought it might be worth putting my notes here to see if any of them need discussion.

• mono and crossgen2 identify loosely by name only and rarely if ever check the types or agruments. This means that they can use any intrinsic regardless of parameter types in some situations like AsRef.
• mono does not currently implement IsNullRef or NullRef, should it? Is there someone on the mono side to ask?
• corelcr and mono do not specify vm intrinsics for Read or Write, Should they be decorated as intrinsics? I think it's misleading to decorate them so i would like to remove the attribute. Should they be implemented as vm intrinsics? (they are implemented in il using other unsafe methods that are vm intrinsic)
• corelib Unsafe c# code contains public void Write<T>(ref byte destination, T value) and T Read(ref byte source)` which are not part of the approved api surface. They have no callers in corelib and only crossgen2 would be able to use them and only because of loose identification any other overload of those two methods. Should these be deleted? Should they be made public and taken through review? Made internal?
• ref T AddByteOffset(ref T source, nuint byteOffset) does not have a coreclr vm intrinsic but is implemented in terms of the other AddByteOffset overload. Should it have a vm intrinsic in coreclr?

The list of methods that need to be added into corelib to contain the complete current set of unsafe methods is:

+public static void Copy<T>(void* destination, ref T source)
+public static void Copy<T>(ref T destination, void* source)
+public static void CopyBlock(void* destination, void* source, uint byteCount)
+public static void CopyBlock(ref byte destination, ref byte source, uint byteCount)
+public static void CopyBlockUnaligned(void* destination, void* source, uint byteCount)
+public static void CopyBlockUnaligned(ref byte destination, ref byte source, uint byteCount)
+public static void InitBlock(void* startAddress, byte value, uint byteCount)
+public static void InitBlock(ref byte startAddress, byte value, uint byteCount)
+public static void InitBlockUnaligned(void* startAddress, byte value, uint byteCount)
+public static ref T Unbox<T>(object box) where T : struct
+public static ref T Add<T>(ref T source, nuint elementOffset)
+public static ref T Subtract<T>(ref T source, int elementOffset)
+public static void* Subtract<T>(void* source, int elementOffset)
+public static ref T Subtract<T>(ref T source, nint elementOffset)
+public static ref T Subtract<T>(ref T source, nuint elementOffset)
+public static ref T SubtractByteOffset<T>(ref T source, nint byteOffset)
+public static ref T SubtractByteOffset<T>(ref T source, nuint byteOffset)

• Do these all need to have vm intrinsics added?
• To all targets CoreCLR CrossGen and Mono? CoreCLR doesn't look too bad once you get past the macro hell, crossgen is fairly simple, mono might cause some problems.

[jkotas]

Thank you for looking into this!

void Write(ref byte destination, T value) and T Read(ref byte source)` which are not part of the approved api surface. They have no callers in corelib

If they are unused, you can just delete them.

implemented in terms of the other overload

This implementation technique will tend to generate lower quality code in some situations. I would avoid this technique and just do the intrinsic expansion for everything. We won't need to have discussion about the perf impact of this technique during PR review.

[marek-safar]

mono does not currently implement IsNullRef or NullRef, should it? Is there someone on the mono side to ask?

It'd make sense to have parity for supported intrinsic on both runtimes (@SamMonoRT can help with implementation)

[stephentoub]

mono does not currently implement IsNullRef or NullRef

Is that not what #64314 fixed?

[Wraith2]

Yes, looks like it was fixed while I was waiting for a merge and I didn't want to pull. I'll update.

[lambdageek]

implemented in terms of the other overload

To contradict @jkotas for Mono I think a simpler C# implementation in terms of another overload is preferable at least initially if we get a smaller/cleaner PR to review.

[Wraith2]

After talking with @lambdageek about this I needed to check the mono interpreter as well as the compiler. This leaves me with a slightly more complex table that shows what will need to be provided to make this work (scroll right):

Method	Location	CoreCLR VM Intrinsic	Crossgen2 Intrinsic	Mono AOT Intrinsic	Mono Interpreter Intrinsic	C# Implementation
void* AsPointer(ref T value)	corelib	METHOD__UNSAFE__AS_POINTER	by name	by name	by name
int SizeOf()	corelib	METHOD__UNSAFE__SIZEOF	by name	by name	by name
T As(object? value) where T : class?	corelib	METHOD__UNSAFE__OBJECT_AS	by name	by name	by name
ref TTo As<TFrom, TTo>(ref TFrom source)	corelib	METHOD__UNSAFE__BYREF_AS	by name	by name	by name
ref T Add(ref T source, int elementOffset)	corelib	METHOD__UNSAFE__BYREF_ADD	by name	by name		ref AddByteOffset(ref source, (IntPtr)(elementOffset * (nint)SizeOf()));
ref T Add(ref T source, IntPtr elementOffset)	corelib	METHOD__UNSAFE__BYREF_INTPTR_ADD	by name	by name		return ref AddByteOffset(ref source, (IntPtr)((nint)elementOffset * (nint)SizeOf()));
void* Add(void* source, int elementOffset)	corelib	METHOD__UNSAFE__PTR_ADD	by name	by name		return (byte*)source + (elementOffset * (nint)SizeOf());
ref T AddByteOffset(ref T source, nuint byteOffset)	corelib		by name	by name	by name	return ref AddByteOffset(ref source, (IntPtr)(void*)byteOffset);
bool AreSame(ref T left, ref T right)	corelib	METHOD__UNSAFE__BYREF_ARE_SAME	by name	by name	by name
bool IsAddressGreaterThan(ref T left, ref T right)METHOD__UNSAFE__BYREF_IS_ADDRESS_GREATER_THAN	corelib	METHOD__UNSAFE__BYREF_IS_ADDRESS_GREATER_THAN	by name	by name
bool IsAddressLessThan(ref T left, ref T right)	corelib	METHOD__UNSAFE__BYREF_IS_ADDRESS_LESS_THAN	by name	by name
void InitBlockUnaligned(ref byte startAddress, byte value, uint byteCount)	corelib	METHOD__UNSAFE__BYREF_INIT_BLOCK_UNALIGNED	by name	by name	by name	for (uint i = 0; i < byteCount; i++) AddByteOffset(ref startAddress, i) = value;
T ReadUnaligned(void* source)	corelib	METHOD__UNSAFE__PTR_READ_UNALIGNED	by name	by name		return Unsafe.As<byte, T>(ref (byte)source);
T ReadUnaligned(ref byte source)	corelib	METHOD__UNSAFE__BYREF_READ_UNALIGNED	by name	by name		Unsafe.As<byte, T>(ref source);
void WriteUnaligned(void* destination, T value)	corelib	METHOD__UNSAFE__PTR_WRITE_UNALIGNED	by name	by name		Unsafe.As<byte, T>(ref (byte)destination) = value;
void WriteUnaligned(ref byte destination, T value)	corelib	METHOD__UNSAFE__BYREF_WRITE_UNALIGNED	by name	by name		Unsafe.As<byte, T>(ref destination) = value;
ref T AddByteOffset(ref T source, IntPtr byteOffset)	corelib	METHOD__UNSAFE__BYREF_ADD_BYTE_OFFSET	by name	by name	by name
T Read(void* source)	corelib		by name	by name		Unsafe.As<byte, T>(ref (byte)source);
void Write(void* destination, T value)	corelib		by name	by name		Unsafe.As<byte, T>(ref (byte)destination) = value;
ref T AsRef(void* source)	corelib	METHOD__UNSAFE__AS_REF_POINTER	by name	by name	by name	ref Unsafe.As<byte, T>(ref (byte)source);
ref T AsRef(in T source)	corelib	METHOD__UNSAFE__AS_REF_IN	by name	by name	by name
IntPtr ByteOffset(ref T origin, ref T target)	corelib	METHOD__UNSAFE__BYREF_BYTE_OFFSET	by name	by name	by name	Unsafe.AsPointer(ref source) == null;
bool IsNullRef(ref T source)	corelib	METHOD__UNSAFE__BYREF_IS_NULL	by name	by name
void SkipInit(out T value)	corelib	METHOD__UNSAFE__SKIPINIT	by name	by name	by name
void Copy(void* destination, ref T source)	il
void Copy(ref T destination, void* source)	il
void CopyBlock(void* destination, void* source, uint byteCount)	il
CopyBlock(ref byte destination, ref byte source, uint byteCount)	il
CopyBlockUnaligned(void* destination, void* source, uint byteCount)	il
CopyBlockUnaligned(ref byte destination, ref byte source, uint byteCount)	il
void InitBlock(void* startAddress, byte value, uint byteCount)	il
InitBlock(ref byte startAddress, byte value, uint byteCount)	il
void InitBlockUnaligned(void* startAddress, byte value, uint byteCount)	il		by name	by name
ref T Unbox(object box)	il
ref T Add(ref T source, nuint elementOffset)	il		by name	by name
ref T Subtract(ref T source, int elementOffset)	il
void* Subtract(void* source, int elementOffset)	il
ref T Subtract(ref T source, nint elementOffset)	il
ref T Subtract(ref T source, nuint elementOffset)	il
ref T SubtractByteOffset(ref T source, nint byteOffset)	il
ref T SubtractByteOffset(ref T source, nuint byteOffset)	il

I think that some of the currently il only items can be implemented in terms of each other, it seems likely that the Subtract overloads for example only need one real implementation with the others calling into it. They may also be implementable using the existing intrinsics like Add. If we can fill in those c# implementations it will lower the work required on the mono side.

I need to know how the current System.Runtime.CompilerServices.Unsafe assembly is going to be handled. If it's going to be moved permanently in-box like System.Buffers was then there is no need for it to be an il project and it can be made a very simple c# project like System.Buffers. If it needs to still be shipped out of box then conditional compilation is going to be needed.
In order to move the Unsafe class into corelib the Unsafe class surface area needs to be moved into the System.Runtime assembly ref, only private assemblies refer to it directly through the private corelib. Do all assemblies that currently refer to the Unsafe assembly need to have their references changed to ensure they go through System.Runtime instead or do we continue to refer to the Unsafe assembly?

My original intention was to do this in a single PR because any half implemented state breaks a lot of things. I'm not sure that's feasible given the amount of changes that need to be made so it may need to be a couple of PRs that are merged at the same time when they're both ready. It isn't going to be a small PR, it's 160+ files just updating the managed private assemblies. It will still need the coreclr and nativeaot work as well as the in-box move and possible changes to any projects.

[stephentoub]

Thanks for working on this.

I need to know how the current System.Runtime.CompilerServices.Unsafe assembly is going to be handled.

Presumably we want to ship a new version of the package that unifies the surface area with what's in corelib, and we no longer have harvesting of previously built assets, so I think we'll need to keep the whole existing implementation around as-is, but with build system changes so that the ilproj isn't used for netcoreapp current and instead for netcoreapp current the Unsafe type is typeforwarded down to corelib. @ericstj, please correct me if I'm wrong.

Do all assemblies that currently refer to the Unsafe assembly need to have their references changed to ensure they go through System.Runtime instead or do we continue to refer to the Unsafe assembly?

That would be preferable where possible, basically just deleting the reference to the Unsafe assembly.

[jkotas]

Presumably we want to ship a new version of the package that unifies the surface area with what's in corelib, and we no longer have harvesting of previously built assets

We should not be adding any new public APIs as part of this work, so there should be nothing to unify.

We won't be able to continue shipping the S.R.CS nuget package once we move the implementation to CoreLib. We may be able to ship a new version of the package in .NET 6, but we will definitely have to stop shipping it in .NET 7 to avoid versioning hell.

I would vote to stop shipping it now to simplify things and switch it to same plan as e.g. System.Buffers as @Wraith2 suggested. S.R.CS is the only package that uses ilproj that is a constant source of issues.

basically just deleting the reference to the Unsafe assembly.

Yes, we should clean this up, but it can wait for follow-up PR.

[stephentoub]

so there should be nothing to unify.

Really? If someone has a reference to the existing nuget package that defines Unsafe, that will be a distinct type from the one exposed from System.Runtime, no? Don't we want such references to start type forwarding instead?

[ericstj]

We should dead-end the package. Think of this in a similar way to System.Memory in 3.0. In 7.0 should ship a System.Runtime.CompilerServices.Unsafe.dll with just type-forwards in net7.0. This will have a higher version than any 6.0 package and will automatically win. We delete all the old configurations from the CSProj and stop shipping the package.

For up-stack projects with TargetFrameworks lower than net7.0 they should <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="$(SystemRuntimeCompilerServicesUnsafeVersion)" /> where SystemRuntimeCompilerServicesUnsafeVersion is the LTS version from the 6.0 codebase.

So to be clear:
System.Runtime.CompilerServices.Unsafe.ilproj gets replaced with System.Runtime.CompilerServices.Unsafe.csproj. This project would just be:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
    <ExcludeResourcesImport>true</ExcludeResourcesImport>
    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
  </PropertyGroup>
  <ItemGroup>
    <ProjectReference Include="$(CoreLibProject)" />
  </ItemGroup>
</Project>

[stephentoub]

We should dead-end the package. Think of this in a similar way to System.Memory in 3.0. In 7.0 should ship a System.Runtime.CompilerServices.Unsafe.dll with just type-forwards in net7.0.

Is this different from what I said? Just trying to fix any mental-model discrepancies for the future.

[jkotas]

Is this different from what I said? Just trying to fix any mental-model discrepancies for the future.

I misunderstood "unifies the surface area with what's in corelib" as adding the few extra corelib APIs from the table above as new public APIs. I think you just meant unifying the type identity via type forwarding. I agree with that.

Also, you said that we'll need to keep the whole existing implementation around as-is. It should not be necessary for dead-ending the package as @ericstj explained.

[stephentoub]

I think you just meant unifying the type identity via type forwarding. I agree with that.

Yes

Also, you said that we'll need to keep the whole existing implementation around as-is. It should not be necessary for dead-ending the package as @ericstj explained.

I'm not clear on how this works. If someone has a project that's multi-targeting net7.0 and netstandard2.0, and they're referencing System.Runtime.CompilerServices.Unsafe, what do they do? Do they need to specify a different version of the nuget package to get the best behavior when compiling for each? The old package won't unify the type identities on net7.0, and the new package wouldn't contain the old implementation that works with netstandard2.0 (if we don't keep it around to ship it nor harvest the previously built assemblies). I'm missing something.

[jkotas]

If someone has a project that's multi-targeting net7.0 and netstandard2.0, and they're referencing System.Runtime.CompilerServices.Unsafe, what do they do?

They will reference System.Runtime.CompilerServices.Unsafe 6.0.0.

When compiling for netstandard2.0, it will work just like it works today.

When compiling for net7.0, there will be duplicate System.Runtime.CompilerServices.Unsafe reference assembly: one from the 6.0 package and one from net7.0 runtime refpack. The system will pick up the one from net7.0 because it has higher version. The reference assembly from the nuget package won't make it to the compiler command line.

[stephentoub]

I see. Ok. Thanks.

[Wraith2]

A few of the intrinsics like Add and Subtract can be implemented using unsafe methods so I've filled those in for non CoreCLR targets as is done in the existing code. Unless managed implementations of any additional methods can be provided this leaves the list of work required for mono as getting rid of the red crosses in this table:

Method	Mono AOT Intrinsic	Mono Interpreter Intrinsic	C# Implementation
void* AsPointer(ref T value)	✔️	✔️
int SizeOf()	✔️	✔️
T As(object? value) where T : class?	✔️	✔️
ref TTo As<TFrom, TTo>(ref TFrom source)	✔️	✔️
ref T Add(ref T source, int elementOffset)	✔️	➖	ref AddByteOffset(ref source, (IntPtr)(elementOffset * (nint)SizeOf()));
ref T Add(ref T source, IntPtr elementOffset)	✔️	➖	return ref AddByteOffset(ref source, (IntPtr)((nint)elementOffset * (nint)SizeOf()));
void* Add(void* source, int elementOffset)	✔️	➖	return (byte*)source + (elementOffset * (nint)SizeOf());
ref T AddByteOffset(ref T source, nuint byteOffset)	✔️	✔️	return ref AddByteOffset(ref source, (IntPtr)(void*)byteOffset);
bool AreSame(ref T left, ref T right)	✔️	✔️
bool IsAddressGreaterThan(ref T left, ref T right)METHOD__UNSAFE__BYREF_IS_ADDRESS_GREATER_THAN	✔️	❌
bool IsAddressLessThan(ref T left, ref T right)	✔️	❌
void InitBlockUnaligned(ref byte startAddress, byte value, uint byteCount)	✔️	✔️	for (uint i = 0; i < byteCount; i++) AddByteOffset(ref startAddress, i) = value;
T ReadUnaligned(void* source)	✔️	➖	return Unsafe.As<byte, T>(ref (byte)source);
T ReadUnaligned(ref byte source)	✔️	➖	Unsafe.As<byte, T>(ref source);
void WriteUnaligned(void* destination, T value)	✔️	➖	Unsafe.As<byte, T>(ref (byte)destination) = value;
void WriteUnaligned(ref byte destination, T value)	✔️	➖	Unsafe.As<byte, T>(ref destination) = value;
ref T AddByteOffset(ref T source, IntPtr byteOffset)	✔️	✔️
T Read(void* source)	✔️	➖	Unsafe.As<byte, T>(ref (byte)source);
void Write(void* destination, T value)	✔️	➖	Unsafe.As<byte, T>(ref (byte)destination) = value;
ref T AsRef(void* source)	✔️	✔️	ref Unsafe.As<byte, T>(ref (byte)source);
ref T AsRef(in T source)	✔️	✔️
IntPtr ByteOffset(ref T origin, ref T target)	✔️	✔️
bool IsNullRef(ref T source)	✔️	❌	Unsafe.AsPointer(ref source) == null;
void SkipInit(out T value)	✔️	✔️
void Copy(void* destination, ref T source)	❌	❌
void Copy(ref T destination, void* source)	❌	❌
void CopyBlock(void* destination, void* source, uint byteCount)	❌	❌
void CopyBlock(ref byte destination, ref byte source, uint byteCount)	❌	❌
void CopyBlockUnaligned(void* destination, void* source, uint byteCount)	❌	❌
void CopyBlockUnaligned(ref byte destination, ref byte source, uint byteCount)	❌	❌
void InitBlock(void* startAddress, byte value, uint byteCount)	❌	❌
void InitBlock(ref byte startAddress, byte value, uint byteCount)	❌	❌
void InitBlockUnaligned(void* startAddress, byte value, uint byteCount)	✔️	❌
ref T Unbox(object box)	❌	❌
ref T Add(ref T source, nuint elementOffset)	✔️	➖	ref AddByteOffset(ref source, (nuint)(elementOffset * (nuint)SizeOf()));
ref T Subtract(ref T source, int elementOffset)	➖	➖	ref SubtractByteOffset(ref source, (IntPtr)(elementOffset * (nint)SizeOf()));
void* Subtract(void* source, int elementOffset)	➖	➖	(byte*)source - (elementOffset * (nint)Unsafe.SizeOf());
ref T Subtract(ref T source, nint elementOffset)	➖	➖	ref SubtractByteOffset(ref source, (IntPtr)((nint)elementOffset * (nint)SizeOf()));
ref T Subtract(ref T source, nuint elementOffset)	➖	➖	ref SubtractByteOffset(ref source, (nuint)(elementOffset * (nuint)Unsafe.SizeOf()));
ref T SubtractByteOffset(ref T source, nint byteOffset)	❌	❌
ref T SubtractByteOffset(ref T source, nuint byteOffset)	➖	➖	ref SubtractByteOffset(ref source, (IntPtr)(void*)byteOffset);

[Wraith2]

How is this going to affect wasm?

[ViktorHofer]

This project would just be:

@ericstj as I just saw this, the ExcludeResourcesImport property isn't defined anywhere anymore. You probably meant OmitResources?

[ericstj]

I can't remember which similar project I looked at when I wrote that. Maybe System.Buffers? The property might not even be required since it doesn't have a resx. Feel free to clean it up if you noticed any issue.