JIT: Loop cloning when the method has GDV skips necessary validity checks

[jakobbotsch]

The following program either hits AV or prints memory from a random heap location. We get rid of the bounds check in the loop in Bar.

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public class Program
{
    public static void Main()
    {
        for (int i = 0; i < 4; i++)
        {
            for (int j = 0; j < 200; j++)
            {
                try
                {
                    Bar(new int[10], new C());
                }
                catch (IndexOutOfRangeException)
                {
                }
            }

            Thread.Sleep(500);
        }

        Bar(new int[1], new C());
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Bar(int[] arr, I iface)
    {
        if (arr == null)
        {
            return;
        }

        iface.Foo();

        int i = 10000000;
        do
        {
            Console.WriteLine(arr[i]);
            i++;
        } while (i < arr.Length);
    }
}

internal interface I
{
    void Foo();
}

internal class C : I
{
    public void Foo()
    {
    }
}

The problem is that we skip the following check when we have GDVs in the method, but then still end up optimizing bounds checks. That's only legal when this check passes.

runtime/src/coreclr/jit/loopcloning.cpp

Line 1893 in 9ca9580

if (!loop.lpTestTree->OperIsCompare() || !(loop.lpTestTree->gtFlags & GTF_RELOP_ZTT))

[ghost]

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

Issue Details

---

The following program either hits AV or prints memory from a random heap location. We get rid of the bounds check in the loop in Bar.

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public class Program
{
    public static void Main()
    {
        for (int i = 0; i < 4; i++)
        {
            for (int j = 0; j < 200; j++)
            {
                try
                {
                    Bar(new int[10], new C());
                }
                catch (IndexOutOfRangeException)
                {
                }
            }

            Thread.Sleep(500);
        }

        Bar(new int[1], new C());
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Bar(int[] arr, I iface)
    {
        if (arr == null)
        {
            return;
        }

        iface.Foo();

        int i = 10000000;
        do
        {
            Console.WriteLine(arr[i]);
            i++;
        } while (i < arr.Length);
    }
}

internal interface I
{
    void Foo();
}

internal class C : I
{
    public void Foo()
    {
    }
}

The problem is that we skip the following check when we have GDVs in the method, but then still end up optimizing bounds checks. That's only legal when this check passes.

runtime/src/coreclr/jit/loopcloning.cpp

Line 1893 in 9ca9580

if (!loop.lpTestTree->OperIsCompare() || !(loop.lpTestTree->gtFlags & GTF_RELOP_ZTT))

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

I'm not sure why we leave this particular validity check up to loop cloning instead of rejecting the induction analysis during loop finding. The results of the induction analysis do not seem useful unless we know the full loop body is protected by the loop test.

[AndyAyersMS]

Probably the case that when I added GDV driven cloning and generalized the cloning conditions I didn't look carefully enough at the hot loop optimizations, so for a pure GDV clone, they can turn on when they shouldn't.