JIT: RBO's utilization of liberal VNs is illegal

[jakobbotsch]

The following program hits the assert when run with DOTNET_JitNoCSE=1.

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

public unsafe class Program
{
    public static void Main()
    {
        C c = new C();
        new Thread(_ => { while (true) { c.X = 0; c.X = 1; } }) { IsBackground = true }.Start();

        while (true)
        {
            IllegalRBO(c, 0);
        }
    }

    public class C
    {
        public int X;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void IllegalRBO(C c, int k)
    {
        int y = c.X;

        if (c.X == k + 1)
        {
            if (y == k + 1)
            {
                if (Id(y) != k + 1)
                {
                    Trace.Fail("Impossible");
                }
            }
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static int Id(int x) => x;
}

RBO eliminates the y == k + 1 branch because it is dominated by c.X == k + 1 with the same liberal VN. However, this is not legal since y and c.X evaluated in the dominating test may not have the same value under data races.

The example requires DOTNET_JitNoCSE because otherwise CSE removes one of the loads of c.X, but naturally CSE could decide not to CSE these for any number of reasons (like register pressure).

cc @dotnet/jit-contrib

[dotnet-policy-service]

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

[AndyAyersMS]

Switching RBO to use conservative VNs leads to fairly widespread size regressions. Hoping we can find something more surgical here.

Diffs are based on 2,600,909 contexts (1,033,923 MinOpts, 1,566,986 FullOpts).

MISSED contexts: base: 1 (0.00%), diff: 1,095 (0.04%)

Overall (+1,731,069 bytes)

Collection	Base size (bytes)	Diff size (bytes)	PerfScore in Diffs
aspnet.run.windows.x64.checked.mch	58,516,182	+75,285	+2.35%
benchmarks.run.windows.x64.checked.mch	8,715,674	+41,549	+3.78%
benchmarks.run_pgo.windows.x64.checked.mch	34,073,075	+37,277	+1.03%
benchmarks.run_tiered.windows.x64.checked.mch	12,350,561	+4,935	+2.86%
coreclr_tests.run.windows.x64.checked.mch	383,054,386	+586,083	+25.46%
libraries.crossgen2.windows.x64.checked.mch	45,420,167	+9,044	+2.90%
libraries.pmi.windows.x64.checked.mch	62,230,977	+129,572	+4.79%
libraries_tests.run.windows.x64.Release.mch	287,866,248	+276,273	+1.72%
libraries_tests_no_tiered_compilation.run.windows.x64.Release.mch	138,650,084	+551,038	+3.56%
realworld.run.windows.x64.checked.mch	13,540,888	+13,467	+3.03%
smoke_tests.nativeaot.windows.x64.checked.mch	5,033,665	+6,546	+3.25%

FullOpts (+1,731,069 bytes)

Collection	Base size (bytes)	Diff size (bytes)	PerfScore in Diffs
aspnet.run.windows.x64.checked.mch	31,436,620	+75,285	+2.35%
benchmarks.run.windows.x64.checked.mch	8,715,252	+41,549	+3.78%
benchmarks.run_pgo.windows.x64.checked.mch	20,287,985	+37,277	+1.03%
benchmarks.run_tiered.windows.x64.checked.mch	3,084,991	+4,935	+2.86%
coreclr_tests.run.windows.x64.checked.mch	116,983,748	+586,083	+25.46%
libraries.crossgen2.windows.x64.checked.mch	45,418,462	+9,044	+2.90%
libraries.pmi.windows.x64.checked.mch	62,117,476	+129,572	+4.79%
libraries_tests.run.windows.x64.Release.mch	109,717,164	+276,273	+1.72%
libraries_tests_no_tiered_compilation.run.windows.x64.Release.mch	128,272,509	+551,038	+3.56%
realworld.run.windows.x64.checked.mch	13,127,635	+13,467	+3.03%
smoke_tests.nativeaot.windows.x64.checked.mch	5,032,618	+6,546	+3.25%

[AndyAyersMS]

Still not sure how to approach this, but RBO's been like this for a while now, so fixing doesn't seem urgent. Going to move out of 9.0.