Move IsKnownConstant handling to ExtendedDefaultPolicy

[EgorBo]

In #63734 I added a special support for IsKnownConstant in the inliner, but I added it to the conservative DefaultPolicy (that doesn't scan call opcodes and is not able to recognize intrinsics). Moving to ExtendedDefaultPolicy (used by default in jit).

The idea of the heuristic is if callee has IsKnownConstant(X) where X is a constant at the callsite in its caller => try harder inlining that callee into caller".

[ghost]

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

Issue Details

---

In #63734 I added a special support for IsKnownConstant in the inliner, but I added it to the conservative DefaultPolicy (that doesn't scan call opcodes and is not able to recognize intrinsics). Moving to ExtendedDefaultPolicy (used by default in jit).

The idea of the heuristic is if callee has IsKnownConstant(X) where X is a constant at the callsite in its caller => try harder inlining that callee into caller".

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

[jkotas]

Moving to ExtendedDefaultPolicy (used by default in jit).

Is this policy used by AOT as well?

[EgorBo]

Moving to ExtendedDefaultPolicy (used by default in jit).

Is this policy used by AOT as well?

Only when --Ot is used at the moment.

[EgorBo]

@dotnet/jit-contrib PTAL, simple change, I need it for my other PR

[am11]

@EgorBo, are these pmi diffs expected:

libraries.pmi.Linux.x64.checked.mch:
...
Top method regressions (bytes):
        9499 (55.42 % of base) : 209928.dasm - Sys:GetDriveType(System.String):int
...
4 total methods with Code Size differences (0 improved, 4 regressed), 0 unchanged.

in this method:

runtime/src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs

Line 78 in 59e643b

private static DriveType GetDriveType(string fileSystemName)

which has a large switch case for variety of filesystems? (probably it is not critical for hot paths, just curious)

[EgorBo]

I've lowered the multiplier.

The problem with IsKnownConstant(arg) that it increases the benefit multiplier if input is a constant - it's great! But in reality we usually have something like this:

if (IsKnownConstant(arg) && arg == 42)

so we should inline only if arg not only a constant at the callsite, but also a specific constant. It's not currently possible to correctly handle it unfortunately - inliner doesn't model basic blocks atm (but eventually will!), only if we introduce RuntimeHelpers.IsKnownTrue(arg == 42) but even that is not enough.

The regression in GetDriveType is rather an improvement I think, String.Equals makes sense to always inline for constant input. But for such a large function we most likely don't inline all of the Equals because of too many locals (e.g. Inliner has a heuristic:

runtime/src/coreclr/jit/inlinepolicy.cpp

Lines 1716 to 1723 in fe24ab3

	// Slow down if there are already too many locals
	if (m_RootCompiler->lvaTableCnt > 64)
	{
	// E.g. MaxLocalsToTrack = 1024 and lvaTableCnt = 512 -> multiplier *= 0.5;
	const double lclFullness = min(1.0, (double)m_RootCompiler->lvaTableCnt / JitConfig.JitMaxLocalsToTrack());
	multiplier *= (1.0 - lclFullness);
	JITDUMP("\nCaller has %d locals. Multiplier decreased to %g.", m_RootCompiler->lvaTableCnt, multiplier);
	}

[EgorBo]

@dotnet/jit-contrib @AndyAyersMS PTAL

[AndyAyersMS]

It seems like if there are many such calls in a method, we might want to be less aggressive overall. Can you see if this is the case in some of the larger regressions?

        3905 (34.53 % of base) : 48452.dasm - NpgsqlConnectionStringBuilder:ToCanonicalKeyword(String):String:this

Right now we evaluate inlinees more or less independently of one another, so recognizing this pattern might not be easy, but it would be interesting to see if there's a way to do this, say at least for the initial root level set of inlinees.

[EgorBo]

@AndyAyersMS I decided to remove benefit multiplier for it, because it already gets additional from "foldable branch" since JIT is able to recognize it.
Also, the existence of of IsKnownConstant currently removes BB count limits.

I don't want to add it additional boost because of:

if (IsKnownConstant(arg) && arg == 42)
{
    // fast path
}

where basically any constant arg will trigger that, but we only care about one specific value.

No diffs.

[AndyAyersMS]

LGTM.