dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.03k stars 4.68k forks source link

Side effects of boxing are getting removed #74635

Closed MichalStrehovsky closed 2 years ago

MichalStrehovsky commented 2 years ago

Debug build of this works. Release build (with tiered compilation off) throws an exception because we transformed the whole body into a throw. Reproed this with Preview 7.

class Program
{
    interface IDevirt
    {
        int GetAndSet(int x);
    }

    struct Devirt : IDevirt
    {
        public int X;

        public int GetAndSet(int x)
        {
            int result = X;
            X = x;
            return result;
        }
    }

    unsafe static void Main()
    {
        int result = ((IDevirt)new Devirt { X = 123 }).GetAndSet(456);
        if (result != 123)
            throw new Exception();
    }
}

As an additional task for myself, I need to figure out why we don't see it in the CI since this is literally a NativeAOT test that we run on every CoreCLR checkin and I can also repro it with NativeAOT when compiling this test out of the tree for a weird thing I'm doing on the side).

https://github.com/dotnet/runtime/blob/62cb5f1eb8e214532eb7448f0a524772d5ea56ea/src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs#L2429-L2435

ghost commented 2 years ago

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

Issue Details
Debug build of this works. Release build (with tiered compilation off) throws an exception because we transformed the whole body into a throw. Reproed this with Preview 7. ```csharp class Program { interface IDevirt { int GetAndSet(int x); } struct Devirt : IDevirt { public int X; public int GetAndSet(int x) { int result = X; X = x; return result; } } unsafe static void Main() { int result = ((IDevirt)new Devirt { X = 123 }).GetAndSet(456); if (result != 123) throw new Exception(); } } ``` As an additional task for myself, I need to figure out why we don't see it in the CI since this is literally a NativeAOT test that we run on every CoreCLR checkin and I can also repro it with NativeAOT when compiling this test out of the tree for a weird thing I'm doing on the side). https://github.com/dotnet/runtime/blob/62cb5f1eb8e214532eb7448f0a524772d5ea56ea/src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs#L2429-L2435
Author: MichalStrehovsky
Assignees: -
Labels: `area-CodeGen-coreclr`
Milestone: -
MichalStrehovsky commented 2 years ago

As an additional task for myself, I need to figure out why we don't see it in the CI since this is literally a NativeAOT test that we run on every CoreCLR checkin and I can also repro it with NativeAOT when compiling this test out of the tree for a weird thing I'm doing on the side)

I suspected that we don't optimize when we should, but we do pass -O to ILC and this is optimized IL.

The generated IL is a little bit different between what I got with Preview 7 vs what I'm getting within the dotnet/runtime tree, so I'm going to chalk it up to the extra temporary:

  .locals init (int32 V_0,
           valuetype Generics/TestDevirtualization/Devirt V_1)
  IL_0000:  ldloca.s   V_1
  IL_0002:  initobj    Generics/TestDevirtualization/Devirt
  IL_0008:  ldloca.s   V_1
  IL_000a:  ldc.i4.s   123
  IL_000c:  stfld      int32 Generics/TestDevirtualization/Devirt::X
  IL_0011:  ldloc.1
  IL_0012:  box        Generics/TestDevirtualization/Devirt
  IL_0017:  ldc.i4     0x1c8
  IL_001c:  callvirt   instance int32 Generics/TestDevirtualization/IDevirt::GetAndSet(int32)
+  IL_0021:  stloc.0
+  IL_0022:  ldloc.0
  IL_0023:  ldc.i4.s   123
  IL_0025:  beq.s      IL_002d
  IL_0027:  newobj     instance void [System.Runtime]System.Exception::.ctor()
  IL_002c:  throw
  IL_002d:  ret
} // end of metho
AndyAyersMS commented 2 years ago

@MichalStrehovsky how did you run across this? Trying to determine if the fix warrants a backport to .NET 7.

MichalStrehovsky commented 2 years ago

@MichalStrehovsky how did you run across this? Trying to determine if the fix warrants a backport to .NET 7.

This was a test that I wrote some time ago. I don't actually know why it's written like that and what I was thinking when I wrote it. It's not customer code. https://github.com/dotnet/corert/pull/7469