JIT optimization removes some necessary code in .NET Core 3.0/3.1

[SergeMatskov]

If the next code will be built in the Release configuration, an exception will be thrown because the object summary is null. This affects only .NET Core 3.0 and later. There is no issue in .NET Core 1.0-2.2.

    public struct Ptr<T>
        where T: class
    {
        private T _value;

        public Ptr(T value)
        {
            _value = value;
        }

        //[MethodImpl(MethodImplOptions.NoInlining)]
        public T Release()
        {
            T tmp = _value;
            _value = null;
            return tmp;
        }
    }

    public class TestClass
    {
    }

    class Program
    {
        private static void Main(string[] args)
        {
            Ptr<TestClass> ptr = new Ptr<TestClass>(new TestClass());

            bool res = false;
            while (res)
            {
            }

            TestClass summary = ptr.Release();

            if (summary == null)
                throw new Exception("JIT optimization failure");
            Consume(summary);
            Console.WriteLine("OK");
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        private static void Consume(TestClass summary)
        { 
        }
    }

There are some ways to fix an issue under .NET Core 3.0:

• Disable optimization in the project settings.
• Remove empty while loop.
• Uncomment MethodImpl attribute on Ptr.Release() method.
• Make Ptr non-generic class (or just make private T _value; member of Ptr class of type object).

Disassembly of JITed code in .NET Core 3.1:

           Ptr<TestClass> ptr = new Ptr<TestClass>(new TestClass());
00007FF7C9420F50  push        rsi  
00007FF7C9420F51  sub         rsp,20h  

            bool res = false;
00007FF7C9420F55  mov         rcx,7FF7C94A4170h  
00007FF7C9420F5F  call        00007FF828F36CB0  
00007FF7C9420F64  mov         rsi,rax  
00007FF7C9420F67  mov         rcx,rsi  
00007FF7C9420F6A  call        00007FF7C9414C10  
00007FF7C9420F6F  mov         ecx,1  
00007FF7C9420F74  mov         rdx,7FF7C94BF788h  
00007FF7C9420F7E  call        00007FF8290605B0  
00007FF7C9420F83  lea         rcx,[rsi+10h]  
00007FF7C9420F87  mov         rdx,rax  
00007FF7C9420F8A  call        00007FF828F35DF0  
                throw new Exception("JIT optimization failure");
00007FF7C9420F8F  mov         rcx,rsi  
00007FF7C9420F92  call        00007FF828EAB900  
00007FF7C9420F97  int         3  

Disassembly of JITed code in .NET Core 2.2:

            Ptr<TestClass> ptr = new Ptr<TestClass>(new TestClass());
00007FF7C9401520  sub         rsp,28h  
00007FF7C9401524  mov         rcx,7FF7C92E6708h  
00007FF7C940152E  call        00007FF828EEB3B0  
00007FF7C9401533  mov         rcx,rax  
00007FF7C9401536  mov         rax,rcx  
            {
            }

            TestClass summary = ptr.Release();
00007FF7C9401539  mov         rcx,rax  
00007FF7C940153C  call        00007FF7C94010A0  
            Console.WriteLine("OK");
00007FF7C9401541  mov         rcx,262B4E23068h  
00007FF7C940154B  mov         rcx,qword ptr [rcx]  
00007FF7C940154E  call        00007FF7C9401398  
00007FF7C9401553  nop  
00007FF7C9401554  add         rsp,28h  
00007FF7C9401558  ret  

The test project was attached:
JitOptimizationFailure.zip

category:correctness
theme:importer
skill-level:intermediate
cost:small

[jashook]

/cc @dotnet/jit-contrib

[AndyAyersMS]

The importer reorders

    T tmp = _value;
    _value = null;

when inlining Release, so summary ends up being null.

Importing BB09 (PC=000) of 'Ptr`1[__Canon][System.__Canon]:Release():System.__Canon:this'
    [ 0]   0 (0x000) ldarg.0
    [ 1]   1 (0x001) ldfld 0A000004
    [ 1]   6 (0x006) ldarg.0
    [ 2]   7 (0x007) ldflda 0A000004
    [ 2]  12 (0x00c) initobj 1B000002

               [000065] IA----------              *  ASG       struct (init)
               [000064] -------N----              +--*  BLK       struct<8>
               [000062] ------------              |  \--*  ADDR      byref 
               [000061] ------------              |     \--*  FIELD     ref    _value
               [000059] ------------              |        \--*  ADDR      byref 
               [000060] ------------              |           \--*  LCL_VAR   struct<Ptr`1[[TestClass]], 8> V01 loc0         
               [000063] ------------              \--*  CNS_INT   int    0

    [ 1]  18 (0x012) ret

    Inlinee Return expression (before normalization)  =>
               [000058] ------------              *  FIELD     ref    _value
               [000056] ------------              \--*  ADDR      byref 
               [000057] ------------                 \--*  LCL_VAR   struct<Ptr`1[[TestClass, ex, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null]], 8> V01 loc0         

This reordering doesn't happen unless Release is inlined; when not inlined, there are exception flags on the byref access.

Looking at what 2.x did now to see why we regressed here.

[AndyAyersMS]

Haven't pinned down exactly where this regressed, though I believe the following changes are related:

• Fix importer spilling in the presence of assignment side effects coreclr#19950
• Struct & SIMD improvements coreclr#22255
• Fix spill check for struct lclVars coreclr#23570

Here's what happens in the importer after creating IR for the initobj:

;; 2.2
at SPILL_APPEND, checks to see if it should call impSpillLclRefs
   but doesn't, as LHS is a BLK
then calls impAppendStmt
   detects non-GC BLK LHS
   sets the global effects flag
then calls impSpillSideEffects (global)
   sees gtHasLocalsWithAddrOp on local 0, spills

;; 5.x (assume 3.x is similar, still need to verify)
at SPILL_APPEND, checks to see if it should call impSpillLclRefs
   but doesn't, seems to not handle fields of local structs
then calls impAppendStmt
   does not see any reason to spill global refs
   so no spilling
==> assignment and field access get incorrectly reordered.

One possible avenue to explore for a fix: because the initobj here is storing to a struct field, so it needs spill processing similar to what we'd do for stfld, which is quite a bit broader than the checks done at SPILL_APPEND. Or we could try and generalize the checks done at SPILL_APPEND to handle fields as was discussed in the review for dotnet/coreclr#23570.

Another oddity here is that the initiobj is actually initializing a ref type field, but the IR treats this as a struct typed field. Not sure if this causes issues, but seems like something we might want to handle differently.

cc @CarolEidt @mikedn

[AndyAyersMS]

Using impIsAddrInLocal in the SPILL_APPEND logic fixes the issue here, and is probably the most surgical fix.

[CarolEidt]

@AndyAyersMS - that makes sense. I assume you mean impIsAddressInLocal in place of the lhs->AsBlk()->Addr()->IsLocalAddrExpr();? Looking at impIsAddressInLocal, it occurs to me that it should also be handling GT_LCL_VAR_ADDR or at least asserting that it isn't called with that.

[mikedn]

Using impIsAddrInLocal in the SPILL_APPEND logic fixes the issue here, and is probably the most surgical fix.

impIsAddrInLocal only recognizes FIELD access, it doesn't recognize ADD. Isn't the current logic kind of backwards - spill if you can prove that it is a local access? Shouldn't it be - spill all unless you can prove that it is a local access and then spill only that local?

Looking at impIsAddressInLocal, it occurs to me that it should also be handling GT_LCL_VAR_ADDR or at least asserting that it isn't called with that.

Not need at the moment as the importer doesn't generate GT_LCL_VAR_ADDR. But there may be a problem in fgMorphOneAsgBlockOp, which wrongly uses impIsAddressInLocal instead of IsLocalAddrExpr. There's a reason LCL_ADDR nodes are currently generated only in very specific cases :)

[mikedn]

Another oddity here is that the initiobj is actually initializing a ref type field, but the IR treats this as a struct typed field. Not sure if this causes issues, but seems like something we might want to handle differently.

I've been looking into that a while ago. In fact I already did it but it results in a small regression due to GC/non-GC type mismatch in LSRA that prevents 0 constant reg reuse and I'm trying to fix that first.

[AndyAyersMS]

I assume you mean impIsAddressInLocal in place of the lhs->AsBlk()-> ...

Yes, something like this:

                    else if (lhs->OperIsBlk())
                    {
                        // Check for ADDR(LCL_VAR), or ADD(ADDR(LCL_VAR),CNS_INT))
                        // (the latter may appear explicitly in the IL).
                        // Local field stores will cause the stack to be spilled when
                        // they are encountered.
                        GenTree* lclTree = nullptr;
                        if (impIsAddressInLocal(lhs->AsBlk()->Addr(), &lclTree))
                        {
                            lclVar = lclTree->AsLclVarCommon();
                        }
                    }

Isn't the current logic kind of backwards

It is certainly convoluted.... It would appear that the SPILL_APPEND check is only supposed to handle special cases that later analysis in impAppendStmt might miss, and prior opcode-specific spill logic hasn't already handled.