JIT: Fix weights in fgOptimizeUncondBranchToSimpleCond

[EgorBo]

Fixes #50419

Repro (COMPlus_TieredPGO=1):

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

class Program
{
    static void Main()
    {
        // Collect some profile, wait for tier0->tier1 promotion.
        for (int i = 0; i < 100; i++)
        {
            Test(new object());
            Thread.Sleep(16);
        }
    }

    static object someObj;

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Test(object o)
    {
        if (o != someObj)
            o = null;

        if (o != null)
            Test1();
    }

    [MethodImpl(MethodImplOptions.NoInlining)] static void Test1() { }
}

Codegen diff between main and this PR: https://www.diffchecker.com/iqSxlzMG (note "edge weights are invalid" on the left)

The edges' weights aren't even rendered on the left (same phase).

[EgorBo]

I've tested this on a heavy desktop app (https://github.com/icsharpcode/AvaloniaILSpy)

Before: 367 methods are reported to have invalid weights
After: 338

(investigating other failures)

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib

[AndyAyersMS]

I think it needs to be more general than this. Try some cases where the condition is not always true or not always false.

We have something like this to begin with. Say we know all the block and edge weights and they are consistent and weight(X) is not zero (if it is we perhaps should not do this opt):

We want to duplicate X into A (expecting that the one of the paths out of A will get optimized away):

So the block weight of X needs to decrease (it was A+B, it should now be just B). The block weight of T is unknowable -- we have to make a guess here. There is no good answer; the simplest is to give the outgoing edges of A the same weight ratio as the outgoing edges of X. Then we're consistent if we don't optimize, but inconsistent if we do; that's ok as it means we've learned something contextual. We'll clean such things up in some future work items.

So the updates should be something like:

ScaleA = weight (A)  / weight(X)
ScaleB = weight (B)  / weight(X)   ;; or 1-ScaleA

weight(X->C) = weight(X->C) * ScaleB
weight(X->D) = weight(X->D) * ScaleB
weight(A->C) = weight(X->C) * ScaleA
weight(A->T)  = weight(X->D) * ScaleA
weight(T) = weight(X->D) * ScaleA
weight(X) = weight(B)

[AndyAyersMS]

Since there can actually be many other preds, a cleaner expression is perhaps:

ScaleA = weight(A) / weight(X)
ScaleNotA = 1 - ScaleA

weight(X)    = weight(X)    * ScaleNotA
weight(X->C) = weight(X->C) * ScaleNotA
weight(X->D) = weight(X->D) * ScaleNotA

weight(A->C) = weight(X->C) * ScaleA
weight(A->T) = weight(X->D) * ScaleA
weight(T)    = weight(X->D) * ScaleA

[EgorBo]

Ping myself

[EgorBo]

@AndyAyersMS could you please take a look, I've integrated your changes

[AndyAyersMS]

Changes LGTM, but there's an assert in crossgen2 to investigate.

[AndyAyersMS]

You probably should watch for division by zero here.

[AndyAyersMS]

Suggested change

	// The optimization doesn't make sense in this case
	if (target->bbWeight == 0.0)
	{
	return false;
	}
	// The optimization doesn't make sense in this case
	if (target->isRunRarely())
	{
	return false;
	}

Also this check is quite cheap so I'd move it up to the top of the method as the first check.

[EgorBo]

Ping myself