Forward substitution for relops can create new data races

[SingleAccretion]

Reproduction:

public class Program
{
    private static int _intStatic;

    private static void Main()
    {
        for (int i = 0; i < Environment.ProcessorCount; i++)
        {
            new Thread(() =>
            {
                while (true)
                {
                    Volatile.Write(ref _intStatic, 0);
                    Volatile.Write(ref _intStatic, 1);
                }
            }).Start();
        }

        while (true)
        {
            Problem(ref _intStatic);
        }
    }
    
    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
    private static void Problem(ref int a)
    {
        var b = Volatile.Read(ref a) == 0;
        var c = b;
        if (b)
        {
            Assert(c);
        }
    }
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Assert(bool b)
    {
        if (!b)
        {
            throw new Exception("!!!");
        }
    }
}

Eventually this program will throw an exception, even as one would think it never should.

The cause is that in the branch forward substitution optimization, we copy the indirection and end up with two indirections (that can, naturally, disagree).

It is unclear to me whether this new behavior can be considered correct or not.

@dotnet/jit-contrib

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

Issue Details

---

Reproduction:

public class Program
{
    private static int _intStatic;

    private static void Main()
    {
        for (int i = 0; i < Environment.ProcessorCount; i++)
        {
            new Thread(() =>
            {
                while (true)
                {
                    Volatile.Write(ref _intStatic, 0);
                    Volatile.Write(ref _intStatic, 1);
                }
            }).Start();
        }

        while (true)
        {
            Problem(ref _intStatic);
        }
    }
    
    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
    private static void Problem(ref int a)
    {
        var b = Volatile.Read(ref a) == 0;
        var c = b;
        if (b)
        {
            Assert(c);
        }
    }
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Assert(bool b)
    {
        if (!b)
        {
            throw new Exception("!!!");
        }
    }
}

Eventually this program will throw an exception, even as one would think it never should.

This cause is that in the branch forward substitution optimization, we copy the indirection and end up with two indirections (that can, naturally, disagree).

It is unclear to me whether this new behavior can be considered correct or not.

@dotnet/jit-contrib

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

[AndyAyersMS]

Thanks for pointing this out. We should be more careful here.