Unsafe.As being intrinsic has exposed a few JIT related issues

[tannergooding]

Issue

#68739 switched several APIs to be intrinsic to the JIT and have the respective IR imported directly by the JIT rather than go through inlining.

While this in general has been positive there have been a couple of issues related to Unsafe.As that have cropped up.

These issues don't appear to be related to the IR being "incorrect" but rather due to less spilling to locals and similar issues causing tree shapes that weren't encountered previously.

This has lead to at least one GC hole being exposed and is potentially related to other issues that have cropped up.

Proposed "Fix"

Given that it is generally desirable for this approach to be used "long term", but the issues that crop up are flaky by nature, it may be beneficial to provide a "switch" that allows this feature to be toggled on/off.

In particular, I propose we either provide a new COMPlus_* switch or FEATURE_* define that allows this functionality to be on by default for debug/checked builds and off by default for release builds.

If a COMPlus_* switch were defined then users could opt-into the functionality in release mode and if we ever changed the default they would have an "out" if any problematic behavior shows up later in an edge case scenario.

This would allow us to continue having the feature and getting failures raised until our confidence is high enough that they have been resolved so this can be enabled in production.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[tannergooding]

CC. @jkotas. Would appreciate your feedback on this in particular and if there is a preference on direction.

[tannergooding]

The proposed switch would impact all System.Runtime.CompilerServices.Unsafe APIs that are treated as JIT intrinsics, not just Unsafe.As.

It would not impact the existing VM or inlining support that we've had for many releases.

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Issue

#68739 switched several APIs to be intrinsic to the JIT and have the respective IR imported directly by the JIT rather than go through inlining.

While this in general has been positive there have been a couple of issues related to Unsafe.As that have cropped up.

These issues don't appear to be related to the IR being "incorrect" but rather due to less spilling to locals and similar issues causing tree shapes that weren't encountered previously.

This has lead to at least one GC hole being exposed and is potentially related to other issues that have cropped up.

Proposed "Fix"

Given that it is generally desirable for this approach to be used "long term", but the issues that crop up are flaky by nature, it may be beneficial to provide a "switch" that allows this feature to be toggled on/off.

In particular, I propose we either provide a new COMPlus_* switch or FEATURE_* define that allows this functionality to be on by default for debug/checked builds and off by default for release builds.

If a COMPlus_* switch were defined then users could opt-into the functionality in release mode and if we ever changed the default they would have an "out" if any problematic behavior shows up later in an edge case scenario.

This would allow us to continue having the feature and getting failures raised until our confidence is high enough that they have been resolved so this can be enabled in production.

Author:	tannergooding
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[jkotas]

I do not think it makes sense to introduce a switch for this. We should just fix the bugs.

We can use [MethodImpl(MethodImplOptions.NoOptimization)] as a temporary workaround if we identify specific methods that have a problem.

[JulieLeeMSFT]

cc @dotnet/jit-contrib.

[tannergooding]

Going to close this as per Jan's comment. Any additional issues should be addressed as they come up.