JIT: Avoid introducing data races in RBO

[jakobbotsch]

RBO can forward sub relops into upcoming conditions. We avoid doing this if the local assigned is live out of the block, but there may still be uses between the def and the condition which could cause us to duplicate reads and introduce data races.

The problem is the same as #62048, which was fixed to handle it for volatile operations. Now that we have an overapproximation of the number of SSA uses stored in their defs we can use this to handle the situation in general. This also means we no longer do the forward sub when there may be multiple uses, which makes sense from a costing perspective.

Fix #78713

[ghost]

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

Issue Details

---

RBO can forward sub relops into upcoming conditions. We avoid doing this if the local assigned is live out of the block, but there may still be uses between the def and the condition which could cause us to duplicate reads and introduce data races.

The problem is the same as #62048, which was fixed to handle it for volatile operations. Now that we have an overapproximation of the number of SSA uses stored in their defs we can use this to handle the situation in general. This also means we no longer do the forward sub when there may be multiple uses, which makes sense from a costing perspective.

Fix #78713

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

[jakobbotsch]

This probably moves some of the regressions I'm seeing in #78698 to this PR.

[SingleAccretion]

Notably, this it is not a 100% fix, since we do not count composite uses.

[jakobbotsch]

Ah, well that's unfortunate as I was trying to simplify the substitution that happens below.
Do we see the composite case here?

[SingleAccretion]

It is theoretically possible, though I suspect quite hard to encounter in practice.

(A reproduction would go along the lines of complex field reinterpretations s.t. we end up with LCL_FLD<parent> as the "unaccounted" use)

[jakobbotsch]

Even if we had accurate SSA counts this still does not solve the problem due to how VN is used by the optimization. For example, the following has only one use of b that has the same VN as the condition, so we end up duplicating a == 0 to the condition and introduce the race again:

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

public class Program
{
    private static int _intStatic;

    private static void Main()
    {
        for (int i = 0; i < 1; 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 = a == 0;
        var c = !Id(!b);
        if (!Id(!c))
        {
            Assert(c);
        }
    }
    
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Assert(bool b)
    {
        if (!b)
        {
            throw new Exception("!!!");
        }
    }

    private static T Id<T>(T val) => val;
}

The Id function is here to avoid Roslyn optimizations and the negations to avoid assertion/copy prop.

In essence it seems like we must disallow duplicating any node that has GTF_GLOB_REF.

[jakobbotsch]

I will close this for now and get Andy's opinion once he's back.