Use argIsInvariant instead of argNode->OperIsConst() for inlining observations.

[sandreenko]

This produces many diffs, the reason is that for a test like this:

class GitHub_43342
{
    public static int Test1(int a, int b)
    {
        if (GetA(a) != GetB(b))
        {
            return 1;
        }
        return 100;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]

    public static int GetA(int a)
    {
        return 1;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]

    public static int GetB(int a)
    {
        return 2;
    }


    public static int Main()
    {
        Test1(1, 2);
        return 100;
    }
}

when we consider to inline Test1 into Main fgFindJumpTargets marks both arguments as CALLSITE_CONSTANT_ARG_FEEDS_TEST and it bumps its inlining score. In this case, the const arguments are not feeding the condition directly and inlining of the Test1 does not help to fold the condition.

Is such behavior desirable or should we improve stack simulation in fgFindJumpTargets so it pops args that are used by calls?

After the change, it also happens for methods like if (callA(this) != callB(this)) because this is an invariant.
It causes more inlining to happen and the diff looks like a size regression:
x64 crossgen: Total bytes of delta: 46090 (0.14% of base)
x64 pmi: Total bytes of delta: 170421 (0.34% of base)

also, we can change the test a little:

class GitHub_43342
{
    public static int Test1(int a, int b)
    {
        int c = 5;
        int d = GetD();
        GetA(a);
        GetB(b);
        if (c != d)
        {
            return 1;
        }
        return 100;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]

    public static int GetA(int a)
    {
        return 1;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]

    public static int GetB(int a)
    {
        return 2;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]

    public static int GetD()
    {
        return 3;
    }


    public static int Main()
    {
        Test1(1, 2);
        return 100;
    }
}

now the constant arguments are not fed to the condition (even indirectly) but the current master still marks Inline candidate has const arg that feeds a conditional. Multiplier increased to 3.

PTAL @AndyAyersMS, does it looks like an issue or is CALLSITE_CONSTANT_ARG_FEEDS_TEST an approximation that should not be precise?

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[EgorBo]

Is argIsInvariant true for noinline call GetA(..) ? 😮 (if yes - what marks it invariant?)

[AndyAyersMS]

CALLSITE_CONSTANT_ARG_FEEDS_TEST an approximation that should not be precise?

As you've observed it is not very precise. What we have now may end up seeing benefits where none exist; that seems tolerable.

Fixing the stack model requires a fair amount of work; currently nothing ever gets popped, so the issues go well beyond calls. But calls are the most complex and costly case -- we'd have to resolve the token to find the call sig to understand the pop/push behavior. So far I've been reluctant to do this because of the potential impact on jit throughput.

[sandreenko]

Is argIsInvariant true for noinline call GetA(..) ? 😮 (if yes - what marks it invariant?)

It is true for Test1(1, 2) arguments when we try to inline Test1 into Main, then fgFindJumpTargets does not see that these values are not used in the condition (they are popped by the GetA, GetB).

[sandreenko]

Ok, so I would like to merge this change because:

1. it simplifies the logic: there is no difference between a const int and a local var address for that logic;
2. it helps with Keep precise argument sizes between import and morph. #43130, now when inlArgInfo[varNum].argNode can be a PUTARG_TYPE I need to check for it each time when I access argNode so a smaller number of accesses - fewer checks;
3. there are methods which benefic from this change, for example in -27 (-13.71% of base) : Microsoft.CodeAnalysis.dasm - SyntaxNodeOrToken:GetDirectives(SyntaxNode,Func2,byref)` we have:

IL_0000  02                ldarg.0     
IL_0001  7b aa 08 00 04    ldfld        0x40008AA
IL_0006  2c 1e             brfalse.s    30 (IL_0026)

that is optimized away when we increase its multiplier because of Inline candidate has const arg that feeds a conditional..

The downside is that it is a code size regression on libraries (as could be expected from additional inlining):

x64 crossgen: 46090 (0.14% of base), (74 improved, 492 regressed)
x64 pmi: 170421 (0.34% of base), (497 improved, 1747 regressed)
arm64 crossgen: 71848 (0.14% of base), (85 improved, 478 regressed)
arm64 pmi: 227832 (0.41% of base), (357 improved, 2373 regressed)
unix x64 crossgen: 72114 (0.16% of base), 85 improved, 476 regressed)
unix x64 pmi: 163349 (0.32% of base), (563 improved, 1683 regressed)

@AndyAyersMS what do you think?
cc @dotnet/jit-contrib

[EgorBo]

Looks like R2R failures are related.

[sandreenko]

Looks like R2R failures are related.

They are most likely caused by #44807

[AndyAyersMS]

Code size increase is a bit more than I'd like, -- would guess many of the new inlines are from the "false positives". But this change makes sense, so let's go ahead and take it.

Maybe getting rid of all these false positives will provide enough benefit to justify a stronger stack model. Opened #45115 so there's a tracking issue.

[AndyAyersMS]

Thanks for spotting this.

[adamsitnik]

@sandreenko your change has most probably improved the performance in DrewScoggins/performance-2#3539 and DrewScoggins/performance-2#3540

[sandreenko]

@sandreenko your change has most probably improved the performance in DrewScoggins/performance-2#3539 and DrewScoggins/performance-2#3540

Cool, thanks for reporting this!