Loop cloning misses definitions for fields of promoted structs

[SingleAccretion]

Reproduction:

using System.Runtime.CompilerServices;

Problem(default);

[MethodImpl(MethodImplOptions.NoInlining)]
static void JitUse<T>(T arg) { }

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static void Problem(ArrayWrapper a)
{
    a = GetArrayLong();

    JitUse(a);
    JitUse(a);

    for (int i = 0; i < 10000; i++)
    {
        a = GetArray();
        JitUse(a.Array[i]);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
static ArrayWrapper GetArray() => new() { Array = new int[0] };

[MethodImpl(MethodImplOptions.NoInlining)]
static ArrayWrapper GetArrayLong() => new() { Array = new int[10000] };

struct ArrayWrapper
{
    public int[] Array;
}

We expect to get an IndexOutOfRangeException here, but instead get an AV.

The cause is that loop cloning thinks that the promoted struct's field is invariant in the loop and deletes the bounds check. The reason it thinks it is invariant is because this is a special case in block morphing, where we leave an independently promoted struct alone, and it looks like optIsVarAssgCB does not account for this. I thus conjecture this will also reproduce for the multi-reg assignment form.

Found while experimenting with morphing of the "one field" case into ASG(LCL_VAR(promoted field), BITCAST(field type(CALL))).

@dotnet/jit-contrib

[ghost]

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

Issue Details

---

Reproduction:

using System.Runtime.CompilerServices;

Problem(default);

[MethodImpl(MethodImplOptions.NoInlining)]
static void JitUse<T>(T arg) { }

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static void Problem(ArrayWrapper a)
{
    a = GetArrayLong();

    JitUse(a);
    JitUse(a);

    for (int i = 0; i < 10000; i++)
    {
        a = GetArray();
        JitUse(a.Array[i]);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
static ArrayWrapper GetArray() => new() { Array = new int[0] };

[MethodImpl(MethodImplOptions.NoInlining)]
static ArrayWrapper GetArrayLong() => new() { Array = new int[10000] };

struct ArrayWrapper
{
    public int[] Array;
}

We expect to get an IndexOutOfRangeException here, but instead get an AV.

The cause is that loop cloning thinks that the promoted struct's field is invariant in the loop and deletes the bounds check. The reason it thinks it is invariant is because this is a special case in block morphing, where we leave an independently promoted struct alone, and it looks like optIsVarAssgCB does not account for this. I thus conjecture this will also reproduce for the multi-reg assignment form.

Found while experimenting with morphing of the "one field" case into ASG(LCL_VAR(promoted field), BITCAST(field type(CALL))).

@dotnet/jit-contrib

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

[EgorBo]

@SingleAccretion is this .NET 6.0 specific?

[SingleAccretion]

I believe not, this sample on sharplab produces similarly incorrect codegen.

Edit: indeed, targeting net5.0, I still see the AV. However, targeting netcoreapp3.1, the reproduction is no more.

[SingleAccretion]

It seems that this problem exists for any case where morph leaves a copy block for promoted destinations. E. g. here's an example for "a custom layout with holes":

using System.Runtime.CompilerServices;

Problem(new() { Index = 0 }, new() { Index = 10000 }, new int[10]);

[MethodImpl(MethodImplOptions.NoInlining)]
public static void JitUse<T>(T arg) { }

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
private static void Problem(StructWithHoles a, StructWithHoles b, int[] d)
{
    var a1 = a;
    var b1 = b;

    for (a1.Index = 0; a1.Index < 10; a1.Index = a1.Index + 1)
    {
        a1 = b1;
        JitUse(d[a1.Index]);
    }
}

[StructLayout(LayoutKind.Explicit)]
struct StructWithHoles
{
    [FieldOffset(0)]
    public int Index;
    [FieldOffset(5)]
    public byte B;
    [FieldOffset(8)]
    public int C;
}

Fatal error. System.AccessViolationException: Attempted to read or write protected memory.

Or another "call" case:

using System.Runtime.CompilerServices;

Problem(new() { Index = 0 }, new int[10]);

[MethodImpl(MethodImplOptions.NoInlining)]
public static void JitUse<T>(T arg) { }

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
private static void Problem(StructWithIndex a, int[] d)
{
    var a1 = a;

    for (a1.Index = 0; a1.Index < 10; a1.Index = a1.Index + 1)
    {
        a1 = GetStructWithIndex();
        JitUse(d[a1.Index]);
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
static StructWithIndex GetStructWithIndex() => new() { Index = 10000 };

struct StructWithIndex
{
    public int Index;
    public int Value;
}

[AndyAyersMS]

@BruceForstall have you looked into this yet? If not, maybe I can take it over.

[AndyAyersMS]

FYI the second and third cases above don't fail with recent main.

I assume they could still be made to fail with suitable tweaks.

[BruceForstall]

@BruceForstall have you looked into this yet? If not, maybe I can take it over.

I haven't. Feel free to take it.

[AndyAyersMS]

Here's a similar case we get wrong:

using System.Runtime.CompilerServices;

return Problem();

[MethodImpl(MethodImplOptions.NoInlining)]
static void JitUse<T>(T arg) { }

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static int Problem()
{
    int[] a = GetArray();
    int[] b = a;

    JitUse(a);
    JitUse(b);

    int r = 0;

    for (int i = 0; i < a.Length; i++)
    {
        a = GetArrayLong();
        r += b[i];
    }

    return r;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int[] GetArray() => new int[] { 1, 2, 3, 4, 90 };

[MethodImpl(MethodImplOptions.NoInlining)]
static int[] GetArrayLong() => new int[10000];

We assume if the iterator var is compared to an array length, that array length is a loop invariant. But it might not be, and we can end up walking off the end of an array with no check.