-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Change ReadOnlySpan indexer to return ref readonly #14727
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CC: @jaredpar |
Have you tested this that it works? I think that this will need adjustments in the JIT and maybe VM as well - because of Span indexer is a very special intrinsic. |
I ran our unit tests locally, which passed (for netfx). What type of testing were you thinking? Peformance? |
That's surprising. Let's wait what the CI says... |
It looks like CI is failing due to missing/mismatching intrinsics. |
Most of the jit code is common for handling Span and ReadOnlySpan indexers. There's just a small difference in how the jit handles these, so yes, it would be easy to make them the same. |
Note, if you end up changing the jit, you probably should also make some changes in the runtime:
Changes in corinfo.h require extra work to propagate to desktop. |
BTW, has the intrinsic already been shipped as part of .NET Core 2.0? If yes, how will the .NET Core 2.0 JIT react if the signature of the method changes in .NET Core 2.1? |
In the shipping product, the jit, the runtime, and corelib all match. So the 2.0 jit will never see the IL from a 2.1 corelib. We (mainly the jit team) sometimes intentionally mismatch parts during development and testing and that's why we have some of these defensive mechanisms around like the jit GUID. |
I would think that if you create a new jit GUID, you should not need to change the intrinsic ID. (Note that this intrinsic is not used by desktop.) |
@AndyAyersMS Ah, I forgot that ReadOnlySpan<T> has a copy in the corlib. But I'm referencing the System.Memory NuGet package, so I believe that my IL will expect a |
@ektrah System.Memory package has not shipped a stable version yet. We are not guaranteeing any sort of backward compatibility for the preview versions. |
Hence my question if the intrinsic has already been shipped in 2.0 or if it will be first ship in 2.1. (Sorry, I'm not fully up to date with the internals.) |
Proposed jit changes here: AndyAyersMS/coreclr@83f166a |
Any ideas what is causing these errors?
Do we need to update something in /vm/mscorlib.h: |
All tests are crashing with access violation. Run any test under debugger (e.g. windbg on Windows). You should see it crash. Share stacktrace of the crash if you are not able to figure out the problem. |
This is another problem (different from the crash). The CoreFX depends on the old method in number of places. Everything that depends on the old method will break. You may need to build a private CoreFX package with the change, publish them to myget and point CoreCLR to this package until the change propagates through the system. Otherwise, the CoreCLR repo is going to be on the floor until the change propagates through the system. |
I do not understand how to break this circular dependency. What change do I need to make to the private CoreFX package? Also, is this the property I need to update (i.e. MicrosoftPrivateCoreFxNETCoreAppPackageVersion) once the private package is up on myget? |
@dotnet-bot test this please |
You may want to chat with @weshaggard on how to best execute this. Also, do not forget to include ProjectN in the plan. |
The stacktrace you have shared does not seem to be related to the problem. You should see the crash even when you run a simple hello world - try to reproduce it on a hello world. |
Wow this is a difficult one to stage. I'm a little nervous about using a private package as it will be hard to keep it in sync. Can you build the corefx changes without the CoreCLR change? or will the same thing happen in the CoreFx repo with tests failing?. I suggest starting with creating the corefx PR as well and then we can decide how to proceed. |
It is already there and it depends on CoreCLR to pass CI: dotnet/corefx#24929 |
I ran the Compilation test (which is HelloWorld) and didn't see an AV exception. Just the MissingMethod exception.
|
The CI failures are on checked build. You may want to try that - there may be JIT bug that repros on checked build only. |
…e ReadOnlySpan indexer to return ref readonly. Related to https://github.com/dotnet/corefx/issues/24105 / dotnet/coreclr#14727. [tfs-changeset: 1683794]
Found the issue: string[] mcFiles = Directory.GetFiles(s_tempDir, "*.mc"); GetFiles ends up calling PathHelpers.GetDirectoryNameNoChecks eventually (along with CombineNoChecksInternal/etc.) which uses the ReadOnlySpan indexer (partially due to the changes in here - dotnet/corefx#25426). These recent changes should resolve this issue: |
@dotnet-bot test this please |
@ahsonkhan Could you please resolve the conflict? |
(And maybe squash to a fewer commits as well.) |
8c398b2
to
7c18942
Compare
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.
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 <[email protected]> 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)
7c18942
to
8bc3964
Compare
@@ -175,7 +175,7 @@ public bool IsEmpty | |||
} | |||
} | |||
#else | |||
public T this[int index] | |||
public ref readonly T this[int index] | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You can move the #if PROJECTN
ifdef back here (so that it looks similar to Span.cs).
@@ -199,17 +205,20 @@ private static void RunTest(string testName) | |||
|
|||
if (Global.IsWindows) | |||
{ | |||
Console.WriteLine("================O=========="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you meant to keep these debug statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Will remove.
We have some JIT Hardware Intrinsic tests failing here: Is this a known issue? Are these failures intermittent? cc @BruceForstall, @CarolEdit?
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
Yes, this is known issue: https://github.com/dotnet/coreclr/issues/15519 |
) * 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 <[email protected]> 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. Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
) * 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 <[email protected]> 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. Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
) * 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 <[email protected]> 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. Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
) * 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 <[email protected]> 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. Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
Fixes https://github.com/dotnet/corefx/issues/24105
Related PR: dotnet/corefx#24929
cc @ektrah, @VSadov, @jkotas, @KrzysztofCwalina