Do not consider constant indirections to be invariant

[jakobbotsch]

It is possible they are constant because they were assigned inside the
loop, so we cannot hoist them out in this situation.

Fix #54118

There are SPMI diffs over in #54118 (comment). Note that the regressions where we no longer hoist a method chunk out of a loop can hopefully be better solved by fixing #55079.

[SingleAccretion]

I think it would be prudent to add the same test for a SIMD type. I suspect we'll need to include GT_OBJ in the condition as well.

[jakobbotsch]

I think it would be prudent to add the same test for a SIMD type. I suspect we'll need to include GT_OBJ in the condition as well.

Yep, you're right, same problem there. I'll have to recollect diffs.

[sandreenko]

I am afraid I still don't understand why we can't hoist it could you please help me?
in your test the ssa before we hoist:

r0 = ref arr[0];
loop
r1 = 1;
if (r1 != 1)
{
   return 101;
}
end loop;

and after the transformation it is

r1 = 1;
loop
if (r1 != 1)
{
   return 101;
}
end loop;

that looks like a valid transformation, is not it?

is the problem that we hoist the indirection but don't assign it to a temp that is used in the loop?

It looks like this comment says that such hoisting is expected and valid, do we need to change it?

runtime/src/coreclr/jit/optimizer.cpp

Lines 6181 to 6184 in 96ef64d

	// On the other hand, it is possible for a SSA def to be inside the loop yet the use to be invariant,
	// if the defining expression is also invariant. In such a case the VN invariance would help but it is
	// blocked by the SSA invariance check.
	isInvariant = isInvariant && IsTreeVNInvariant(tree);

[jakobbotsch]

I am afraid I still don't understand why we can't hoist it could you please help me?
in your test the ssa before we hoist:

r0 = ref arr[0];
loop
r1 = 1;
if (r1 != 1)
{
   return 101;
}
end loop;

and after the transformation it is

r1 = 1;
loop
if (r1 != 1)
{
   return 101;
}
end loop;

that looks like a valid transformation, is not it?

I talked with @sandreenko offline, but to recap, we end up with

r1 = ref arr[0];
r2 = *r1;
loop
*r1 = 1;
if (r2 != 1)
  return 101;
end loop

This would be fine if we then also did the substitution of *r1 with the constant, but we don't.
I tried implementing this but ended up with some issues, so I just did the conservative thing.

[SingleAccretion]

Another case where this could manifest is with GT_ARR_LENGTH, since that's too an implicit indirection which VN could understand to be constant. It cannot trigger this bug today though, because VN does not fold ArrLen(JitNewArr(length)) into just ArrLen() + appropriate exception set.

[jakobbotsch]

We can also get it with InitVal:

// Generated by Fuzzlyn v1.2 on 2021-07-03 00:39:06
// Seed: 5880924002759289585
// Reduced from 216.1 KiB to 0.3 KiB in 00:01:57
// Debug: Outputs 0
// Release: Outputs 1
public class Program
{
    public static void Main()
    {
        M22(0);
    }

    static void M22(long arg1)
    {
        long[] var1 = new long[]{1};
        for (int var2 = 0; var2 < 2; var2++)
        {
            var1[0] = arg1;
            long var3 = var1[0];
            System.Console.WriteLine(var3);
        }
    }
}

I have some other cases that I haven't had time to look at yet, e.g.

// Generated by Fuzzlyn v1.2 on 2021-07-03 15:22:25
// Seed: 6511959640880916131
// Reduced from 141.0 KiB to 0.4 KiB in 00:01:11
// Debug: Outputs 1
// Release: Outputs 0
public class Program
{
    static short s_12;
    static ulong s_13;
    public static void Main()
    {
        var vr3 = new short[]{0};
        short vr5 = s_12;
        for (int vr7 = 0; vr7 < 2; vr7++)
        {
            if (s_13 != 1)
            {
                System.Console.WriteLine(vr3[0]);
            }

            vr3[0] = (short)(vr5 + 1);
            vr3[0] = vr3[0];
        }
    }
}

// Generated by Fuzzlyn v1.2 on 2021-07-03 06:25:25
// Seed: 13567327556398074305
// Reduced from 33.5 KiB to 0.3 KiB in 00:00:14
// Debug: Outputs 1
// Release: Outputs 0
public class Program
{
    static uint[] s_1 = new uint[]{1};
    public static void Main()
    {
        var vr2 = (sbyte)s_1[0];
        var vr3 = new sbyte[]{0};
        for (int vr6 = 0; vr6 < 2; vr6++)
        {
            vr3[0] = vr2;
            sbyte vr7 = vr3[0];
            System.Console.WriteLine(vr7);
        }
    }
}

// Generated by Fuzzlyn v1.2 on 2021-07-03 03:31:07
// Seed: 7814308326432864559
// Reduced from 138.9 KiB to 0.4 KiB in 00:00:51
// Debug: Outputs 1
// Release: Outputs 0
public class Program
{
    static byte[] s_6 = new byte[]{0};
    static ushort s_54 = 1;
    public static void Main()
    {
        byte vr0 = (byte)s_54;
        byte[] vr1 = s_6;
        for (int vr2 = 0; vr2 < 2; vr2++)
        {
            vr1[0] = vr0;
            vr1[0] = vr1[0];
        }

        System.Console.WriteLine(vr1[0]);
    }
}

[jakobbotsch]

This fix does not seem sufficient due to those other cases. In addition, we still have the same problem for GT_CLS_VAR for those other cases, e.g.

class Program
{
    static int _a;

    static void Main()
    {
        _a = 1;

        Test(1, 2);
    }

    static void Test(int one, int two)
    {
        while (_a == one)
        {
            System.Console.WriteLine(_a);
            _a = two;
        }
    }
}

results in an infinite loop.

[jakobbotsch]

I'll close this for now as the fix probably needs to be more general.