Fix STOREIND optimization.

[sandreenko]

dotnet/wpf#3226 exposed a scenario when optimization from #38316 did not work correctly.

That PR adds a repro case for that behavior (the scenario requires very specific conditions to expose the problem) and a fix.

In short we need to get a tree like:

               [000008] -A-XG-------              *  ASG       struct (copy)
               [000007] -------N----              +--*  BLK       struct<8>
                                                                     \--* smth
               [000004] ---XG--N----              \--*  FIELD     byte   a

where FIELD byte survives lclMorph and globalMorph, in order to do that the dest struct has to be non-promotable (in my repro >4 fields) and FIELD has to be in the form of

               [000004] ---XG--N----              \--*  FIELD     byte   a
               [000003] *--XG-------                 \--*  IND       long
               [000002] ------------                    \--*  LCL_VAR   long   V00 arg0

(otherwise, lclMorph optimizes it to a LCL_FLD byte and then fgMorphBlockOperand creates a correct IND struct(ADDR(LCL_FLD byte)) on top of it.

if these conditions were met we could have had an IR like (after global morph):

               [000008] -A-XG+------              *  ASG       struct (copy)
               [000000] D----+-N----              +--*  LCL_VAR   struct<B, 8> V02 loc1
               [000004] ---XG+-N----              \--*  IND       byte
               [000003] *--XG+------                 \--*  IND       long   Zero Fseq[a]
               [000002] -----+------                    \--*  LCL_VAR   long   V00 arg0

that was transformed into:

N003 (  7,  5) [000004] ---XG--N----         t4 = *  IND       byte   <l:$283, c:$282>
                                                  /--*  t4     byte
N005 ( 11,  8) [000008] DA-XG-------              *  STORE_LCL_VAR struct<B, 8> V02 loc1         d:2

and then the added optimization was transforming it into:

N003 (  7,  5) [000004] ---XG--N----         t4 = *  IND       byte   <l:$283, c:$282>
               [000013] D------N----        t13 =    LCL_VAR_ADDR byref  V02 loc1
                                                  /--*  t13    byref
                                                  +--*  t4     byte
N005 ( 11,  8) [000008] -A----------              *  STOREIND  long

without the added optimization we also ended with a wrong IND byte, but we always mark STORE_OBJ src as contained, so we did not put it in a register and STORE_OBJ was reading the number of bytes written in DST, not in SRC.

Note: I did not have a chance to shrink the repro and fix names etc, will do tomorrow.

[sandreenko]

PTAL @AndyAyersMS @BruceForstall. I have constructed a repro for the issue that we have discussed earlier. I added a fix but have not tested it fully, so probably we will see ci failures due to new asserts in lowering, but I believe my fix in fgMorphBlockOperand is correct and sufficient.

cc @dotnet/jit-contrib

[AndyAyersMS]

What would be the impact of the simple fix where we just disable Lowering::TryTransformStoreObjAsStoreInd , while we iterate on a better fix?

[BruceForstall]

A somewhat related question: should Lowering::TryTransformStoreObjAsStoreInd only run when we're optimizing, and not in MinOpts?

[AndyAyersMS]

Here's a C# repro case based on the WPF code:

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

[StructLayout(LayoutKind.Sequential)]
class POINT 
{
    public int x;
    public int y;
    public override string ToString() => $"{{{x}, {y}}}";
}

[StructLayout(LayoutKind.Sequential)]
class MINMAXINFO 
{
    public POINT ptMinTrackSize = new POINT();
    public POINT ptMaxTrackSize = new POINT();
}

class Test
{
    static void WmGetMinMaxInfo(IntPtr lParam)
    {
        MINMAXINFO mmi = new MINMAXINFO();

        mmi.ptMinTrackSize.x = 100101;
        mmi.ptMinTrackSize.y = 102103;
        mmi.ptMaxTrackSize.x = 200201;
        mmi.ptMaxTrackSize.y = 202203;

        Marshal.StructureToPtr(mmi, lParam, true);
    }

    public unsafe static int Main()
    {
        MINMAXINFO mmi = new MINMAXINFO();
        IntPtr pmmi = Marshal.AllocHGlobal(Marshal.SizeOf(mmi));
        WmGetMinMaxInfo(pmmi);
        mmi = (MINMAXINFO) Marshal.PtrToStructure(pmmi, typeof(MINMAXINFO));
        bool valid =  mmi.ptMinTrackSize.x == 100101 &&  mmi.ptMinTrackSize.y == 102103 &&  mmi.ptMaxTrackSize.x == 200201 && mmi.ptMaxTrackSize.y == 202203;
        if (!valid)
        {
            Console.WriteLine($"Got {mmi.ptMinTrackSize}, expected {{100101, 102103}}");
            Console.WriteLine($"Got {mmi.ptMaxTrackSize}, expected {{200201, 202203}}");
        }
        return valid ? 100 : -1;
    }
}

This currently prints

Got {5, 0}, expected {100101, 102103}
Got {9, 0}, expected {200201, 202203}

The bad codegen is in ILStubClass.IL_STUB_StructMarshal, which is a marshalling stub created by Marshal.StructureToPtr.

[mangod9]

what was the impact of this issue on the WPF case, was it that the double values expected by SetWindowPos incorrect? When Aaron and I were investigating it appeared that the right size values were present, unless they were somehow misaligned.

[AndyAyersMS]

I think it's that WPF handles WM_GETMINMAXINFO and in doing so, updates the MINMAXINFO, and that update was not happening correctly because of the optimization mentioned here. So it is a bug in the jit codegen for struct marshalling.

[sandreenko]

what was the impact of this issue on the WPF case, was it that the double values expected by SetWindowPos incorrect? When Aaron and I were investigating it appeared that the right size values were present, unless they were somehow misaligned.

we had a copy of 8 bytes from a source to a destination, and the first field in the source was ubyte, so instead of copying 8 bytes from source address the Jit was taking this field, sign extended it and copied that value to the dst. So we were losing 7 bytes of data.

[AndyAyersMS]

Right, the 8 byte copies were the various POINT fields of the MINMAXINFO struct, and we were only copying 1 byte for each.

This struct describes the window size, so as a result the window size was messed up.

[BruceForstall]

LGTM. Thanks for adding the tests.

Consider triggering jitstress job.

[AndyAyersMS]

Looks good.

Can you open a follow-up issue for the more general fix?

I wonder if we should do some extra testing on WPF for this. I will look into it.

[sandreenko]

Consider triggering jitstress job.

Triggered, will wait for it to finish before merging this.

Can you open a follow-up issue for the more general fix?

Sure, was planning to do so.

[AaronRobinsonMSFT]

/cc @jkoritzinsky

[AndyAyersMS]

@sandreenko the newly added IL test is failing with asserts / invalid IL.

[sandreenko]

@sandreenko the newly added IL test is failing with asserts / invalid IL.

Will drop the test and keep your test for this PR, interesting that we have 3 results for the IL test: 64bit platforms: passing, 32bit platforms: morph assert, mono: invalid IL.