Stop retyping RHSes to byrefs on type mismatch (dotnet#58297)

* Stop retyping RHSes to byrefs on type mismatch

This is an interesting piece of code. Using it, it is possible
to give TYP_BYREF to any node that would ordinarily have
TYP_I_IMPL. In particular, I encountered it in my GT_CAST work:
we have tests in the SPMI collections with CAST(byref <- long <- int)
in them. Ordinarily such a tree would assert in VN, but in that
case optimizations were off and so everything went smoothly.

It is possible to have TYP_BYREF UMOD/MUL_OVF_UN and similar
with this code. Naturally, even though this is legal IL, the
compiler does not like it and fails with asserts in multiple
places.

So, just delete the retyping. The cited assert either no longer
exists (though I had a look at the legacy codegen and could not
find it there...) or got reworked at some point.

There are GC Info diffs with this change. They consist of cases
when a constant zero node was retyped to TYP_BYREF and thus its
register's liveness was recorded.

There are few code size diffs with this change. It does pessimize VN
a little because we will now have more cases where a cast VN is added
for an assignment, but VN is already very pessimistic for byrefs, so
it seems acceptable.

* Add a test covering the discovered problem

VN-based copy propagation checks the compatibility of types using
the type of node at use and the RHS of the new local's def. Thus,
when we retype a TYP_I_IMPL RHS to TYP_BYREF, we risk substituting a
TYP_BYREF local with a TYP_I_IMPL one, dangerously shortening the
lifetime of the managed pointer.

src/coreclr/jit/importer.cpp

@@ -12064,15 +12064,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                 }
                 else
                 {
-                    // The code generator generates GC tracking information
-                    // based on the RHS of the assignment.  Later the LHS (which is
-                    // is a BYREF) gets used and the emitter checks that that variable
-                    // is being tracked.  It is not (since the RHS was an int and did
-                    // not need tracking).  To keep this assert happy, we change the RHS
-                    if (lclTyp == TYP_BYREF && !varTypeIsGC(op1->gtType))
-                    {
-                        op1->gtType = TYP_BYREF;
-                    }
                     op1 = gtNewAssignNode(op2, op1);
                 }
 

src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.il

@@ -0,0 +1,155 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+.assembly extern System.Console { auto }
+.assembly extern System.Runtime { auto }
+
+.assembly CopyPropByrefToNativeInt { }
+
+.typedef [System.Runtime]System.WeakReference as WeakRef
+.typedef [System.Runtime]System.GC as GC
+
+.class Program extends [System.Runtime]System.Object
+{
+  .field private static class WeakRef s_weakArrRef
+  .field private static bool s_arrIsAlive
+
+  .method private static int32 Main() cil managed
+  {
+    .entrypoint
+    .locals ( [0] int32 result, [1] int32[] arr )
+
+    ldc.i4 1
+    ldc.i4 1  // The array will be pinned.
+    call !!0[] GC::AllocateArray<int32>(int32, bool)
+    stloc arr
+
+    ldloc arr
+    newobj instance void WeakRef::.ctor(object)
+    stsfld class WeakRef Program::s_weakArrRef
+
+    ldc.i4 1
+    stsfld bool Program::s_arrIsAlive
+
+    ldloc arr
+    ldc.i4 0
+    ldelema int32
+    dup
+    ldc.i4 1
+    call unsigned int8& Program::Problem(unsigned int8&, native int, int32)
+    brtrue SKIP
+
+    // Unreachable, exists to not have "arr"'s liveness extended.
+    call unsigned int8& Program::SideEffect()
+    pop
+
+  SKIP:
+    ldsfld bool Program::s_arrIsAlive
+    brtrue SUCCESS
+
+    ldc.i4 101
+    ret
+
+  SUCCESS:
+    ldc.i4 100
+    ret
+  }
+
+  .method private static unsigned int8& Problem(unsigned int8& byrefAddrArg, native int nintAddr, int32 notZeroArg) cil managed noinlining
+  {
+    .locals ( [0] int32 i, [1] native int nintAddrCopy, [2] unsigned int8& byrefAddr )
+
+    ldarg nintAddr
+    stloc byrefAddr
+
+    // Before this point, array is kept alive by "byrefAddrArg".
+    ldarg byrefAddrArg
+    call void Program::Use(unsigned int8&)
+    // Now the array should be kept alive by "byrefAddr".
+
+    ldc.i4 0
+    stloc i
+  LOOP:
+    ldloc i
+    ldc.i4 1
+    add
+    stloc i
+
+    call void Program::TryCollectArr()
+
+    ldloc i
+    ldc.i4 10
+    ble LOOP
+
+    ldarg nintAddr
+    stloc nintAddrCopy
+
+    ldarg notZeroArg
+    ldc.i4 0
+    bne.un SKIP
+
+    // This path is unreachable and only exists so
+    // that local copy propagation does not substitute
+    // "nintAddrCopy" with "nintAddr" too early. We
+    // want the fact they're equal to be discovered in VN.
+    call unsigned int8& Program::SideEffect()
+    ret
+
+  SKIP:
+    // The below sequence is meant to reduce to just "ldloc byrefAddr; ret;"
+    // It exists because for copy propagation to perform the (wrong) substituion
+    // of "byrefAddr" with "nintAddr", we need to make it seem that "nintAddr"
+    // is live at the point of substition (#Use), so we fake it up with some
+    // instructions that VN knows will evaluate to nothing, but it'll take
+    // until constant VN-based propagation for that fact to be acted on.
+
+    ldarg nintAddr
+    ldloc nintAddrCopy
+    ceq
+    ldc.i4 0
+    ceq
+    conv.u
+
+    ldloc byrefAddr // #Use
+    ldarg nintAddr
+    ldloc nintAddrCopy
+    ceq
+    ldc.i4 0
+    ceq
+    conv.u
+    add
+
+    add
+    ret
+  }
+
+  .method private static void TryCollectArr() cil managed noinlining
+  {
+    .locals ( [0] int32 arrValue )
+
+    ldc.i4 2
+    ldc.i4 1 // GCCollectionMode.Forced
+    call void GC::Collect(int32, valuetype [System.Runtime]System.GCCollectionMode)
+    call void GC::WaitForPendingFinalizers()
+
+    ldsfld class WeakRef Program::s_weakArrRef
+    callvirt instance bool WeakRef::get_IsAlive()
+    ldsfld bool Program::s_arrIsAlive
+    and
+    stsfld bool Program::s_arrIsAlive
+
+    ret
+  }
+
+  .method private static unsigned int8& SideEffect() cil managed noinlining
+  {
+    ldc.i4 0
+    conv.u
+    ret
+  }
+
+  .method private static void Use(unsigned int8& byref) cil managed noinlining
+  {
+    ret
+  }
+}

src/tests/JIT/Methodical/Coverage/copy_prop_byref_to_native_int.ilproj

@@ -0,0 +1,12 @@
+<Project Sdk="Microsoft.NET.Sdk.IL">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).il" />
+  </ItemGroup>
+</Project>