Refactor Unsafe.cs to get it more in sync with CoreRT. #15510
Conversation
|
Could you please move Unsafe.cs to the shared CoreLib partition?
|
Is it OK to change the exception being thrown in either one to match the other? If so, which one is better - InvalidOperationException or PlatformNotSupportedException?
I will do that after this change is merged and the CI on corert side passes. |
When the mirror PR gets created, should we also delete the https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/Internal/Runtime/CompilerServices/Unsafe.cs file at the same time? |
|
Right, we will append the delete to the mirror PR. |
| using nuint = System.UInt64; | ||
| #if PROJECTN |
There was a problem hiding this comment.
This ifdef is not necessary. It does not hurt for nint to be defined for all.
| } | ||
|
|
||
| #if PROJECTN |
There was a problem hiding this comment.
We should keep the same public surface between both ProjectN and CoreCLR.
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds an element offset to the given reference. | ||
| /// </summary> | ||
| [CLSCompliant(false)] | ||
| [Intrinsic] | ||
| [NonVersionable] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ref T AddByteOffset<T>(ref T source, nuint byteOffset) |
There was a problem hiding this comment.
This method is outlier - it is not in the official S.R.CS.Unsafe. It should stay internal. We want the public surface to be the same between 32-bit and 64-bit versions. nuint is changing between 32-bit and 64-bit versions.
| @@ -94,117 +107,249 @@ public static int SizeOf<T>() | |||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public static ref T Add<T>(ref T source, int elementOffset) | |||
| { | |||
| #if PROJECTN | |||
| return ref AddByteOffset(ref source, (IntPtr)(elementOffset * (nint)SizeOf<T>())); | |||
| #else | |||
| // The body of this function will be replaced by the EE with unsafe code!!! | |||
| // See getILIntrinsicImplementationForUnsafe for how this happens. | |||
| typeof(T).ToString(); // Type token used by the actual method body | |||
There was a problem hiding this comment.
These methods with typeof(T).ToString() in them should be rather ifdefed for CoreCLR, like:
#if CORECLR
typeof(T).ToString();.....
#else
...
#endif
| [NonVersionable] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ref T AddByteOffset<T>(ref T source, nuint byteOffset) | ||
| { | ||
| #if PROJECTN |
There was a problem hiding this comment.
This ifdef is not necessary. The ProjectN implementation will work for CoreCLR as well.
| [NonVersionable] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static void InitBlockUnaligned(ref byte startAddress, byte value, uint byteCount) | ||
| { | ||
| #if PROJECTN | ||
| for (uint i = 0; i < byteCount; i++) |
There was a problem hiding this comment.
This impl works for CoreCLR as well.
You can append more changes to the file on the corert side if there are problems. The file can be moved right away. |
| [Intrinsic] | ||
| [NonVersionable] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| internal static T Read<T>(void* source) |
There was a problem hiding this comment.
These can be public. (I think that actually have to be public - it would cause a build break in ProjectN otherwise.)
There was a problem hiding this comment.
And the implementation can remain common between CoreCLR/ProjectN?
There was a problem hiding this comment.
Yes. (Note that it does not prevent the runtime to provide more streamlined implementation.)
| /// </summary> | ||
| [NonVersionable] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| internal static IntPtr ByteOffset<T>(ref T origin, ref T target) |
There was a problem hiding this comment.
This has to be public too - it is needed by System.Memory overlapped, right?
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| internal static ref T AddByteOffset<T>(ref T source, IntPtr byteOffset) | ||
| { | ||
| // This method is implemented by the toolchain |
There was a problem hiding this comment.
We should just one unified version of the comment at the top of the file. Something like:
The implementations of most the methods in this file are provided as intrinsics.
In CoreCLR, see getILIntrinsicImplementationForUnsafe.
In CoreRT, see Internal.IL.Stubs.UnsafeIntrinsics.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ref T AsRef<T>(void* source) | ||
| { | ||
| #if CORECLR |
There was a problem hiding this comment.
Should it just throw?
No method body?
There was a problem hiding this comment.
The !CORECLR implementation can be used everywhere.
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static int SizeOf<T>() | ||
| { | ||
| typeof(T).ToString(); // Type token used by the actual method body |
There was a problem hiding this comment.
typeof(T).ToString(); may be under CORECLR ifdef (since all of the other ones are).
|
LGTM otherwise. |
| /// <summary> | ||
| /// Adds an element offset to the given reference. | ||
| /// </summary> | ||
| [NonVersionable] |
There was a problem hiding this comment.
Could you please add [Intrinsic] to all the methods in the file for consistency?
Reverting the change to Enumerator Current for now Fix file formatting Enable Alpine CI (dotnet#15502) * Enable Alpine CI This enables Alpine CI leg on every PR using the pipelines. compare type size instead of var_types get rid of TYP_CHAR Adding support for Acosh, Asinh, Atanh, and Cbrt to Math and MathF Updating the PAL layer to support acosh, asinh, atanh, and cbrt Adding some PAL tests for acosh, asinh, atanh, and cbrt Adding valuenum support for acosh, asinh, atanh, and cbrt Lsra Documentation Update LinearScan section of ryujit-overview.md, and add lsra-detail.md Refactor Unsafe.cs to get it more in sync with CoreRT. (dotnet#15510) * Refactor Unsafe.cs to get it more in sync with CoreRT. * Format the document. * Unifying the copies of Unsafe using ifdefs * Change exception thrown to PlatformNotSupportedException * Addressing PR feedback and moving Unsafe to shared. * Addressing PR feedback * Addressing PR review - adding intrinsic attribute Update CoreClr, CoreFx to preview1-26014-01, preview1-26013-12, respectively (dotnet#15513) Revert "Add optional integer offset to OwnedMemory Pin (dotnet#15410)" This reverts commit 8931cfa. Get rid of old -altjitcrossgen argument now that CI has been updated Merge pull request dotnet/corert#5109 from dotnet/nmirror (dotnet#15518) Merge nmirror to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (dotnet#14982)"" Fix typo `_TARGET_ARM` to `_TARGET_ARM_`. This happens mostly in comments except lsra.cpp. Update CoreClr, CoreFx, PgoData to preview1-26014-04, preview1-26014-03, master-20171214-0043, respectively (dotnet#15520)
Reverting the change to Enumerator Current for now Fix file formatting Enable Alpine CI (dotnet#15502) * Enable Alpine CI This enables Alpine CI leg on every PR using the pipelines. compare type size instead of var_types get rid of TYP_CHAR Adding support for Acosh, Asinh, Atanh, and Cbrt to Math and MathF Updating the PAL layer to support acosh, asinh, atanh, and cbrt Adding some PAL tests for acosh, asinh, atanh, and cbrt Adding valuenum support for acosh, asinh, atanh, and cbrt Lsra Documentation Update LinearScan section of ryujit-overview.md, and add lsra-detail.md Refactor Unsafe.cs to get it more in sync with CoreRT. (dotnet#15510) * Refactor Unsafe.cs to get it more in sync with CoreRT. * Format the document. * Unifying the copies of Unsafe using ifdefs * Change exception thrown to PlatformNotSupportedException * Addressing PR feedback and moving Unsafe to shared. * Addressing PR feedback * Addressing PR review - adding intrinsic attribute Update CoreClr, CoreFx to preview1-26014-01, preview1-26013-12, respectively (dotnet#15513) Revert "Add optional integer offset to OwnedMemory Pin (dotnet#15410)" This reverts commit 8931cfa. Get rid of old -altjitcrossgen argument now that CI has been updated Merge pull request dotnet/corert#5109 from dotnet/nmirror (dotnet#15518) Merge nmirror to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (dotnet#14982)"" Fix typo `_TARGET_ARM` to `_TARGET_ARM_`. This happens mostly in comments except lsra.cpp. Update CoreClr, CoreFx, PgoData to preview1-26014-04, preview1-26014-03, master-20171214-0043, respectively (dotnet#15520)
* Change ReadOnlySpan indexer to return ref readonly. Update JIT to handle changes to ReadOnlySpan indexer Resolving merge conflict and fixing jit importer Update ReadOnlySpan Enumerator Current to use indexer. Removing readonly keyword. * Temporarily disabling Span perf and other tests that use ReadOnlySpan * Isolating the ref readonly indexer change only to CoreCLR for now. Reverting the change to Enumerator Current for now Fix file formatting Enable Alpine CI (#15502) * Enable Alpine CI This enables Alpine CI leg on every PR using the pipelines. compare type size instead of var_types get rid of TYP_CHAR Adding support for Acosh, Asinh, Atanh, and Cbrt to Math and MathF Updating the PAL layer to support acosh, asinh, atanh, and cbrt Adding some PAL tests for acosh, asinh, atanh, and cbrt Adding valuenum support for acosh, asinh, atanh, and cbrt Lsra Documentation Update LinearScan section of ryujit-overview.md, and add lsra-detail.md Refactor Unsafe.cs to get it more in sync with CoreRT. (#15510) * Refactor Unsafe.cs to get it more in sync with CoreRT. * Format the document. * Unifying the copies of Unsafe using ifdefs * Change exception thrown to PlatformNotSupportedException * Addressing PR feedback and moving Unsafe to shared. * Addressing PR feedback * Addressing PR review - adding intrinsic attribute Update CoreClr, CoreFx to preview1-26014-01, preview1-26013-12, respectively (#15513) Revert "Add optional integer offset to OwnedMemory Pin (#15410)" This reverts commit 8931cfa. Get rid of old -altjitcrossgen argument now that CI has been updated Merge pull request dotnet/corert#5109 from dotnet/nmirror (#15518) Merge nmirror to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Revert " Revert "[Local GC] Move knowledge of overlapped I/O objects to the EE through four callbacks (#14982)"" Fix typo `_TARGET_ARM` to `_TARGET_ARM_`. This happens mostly in comments except lsra.cpp. Update CoreClr, CoreFx, PgoData to preview1-26014-04, preview1-26014-03, master-20171214-0043, respectively (#15520) * Disabling a test that uses ReadOnlySpan indexer * Temporarily disabling the superpmi test and fixing nit * Remove debug statements.
Related to: dotnet/corert#5105
cc @jkotas, @sergiy-k, @KrzysztofCwalina