Don't inline StelemRef_Helper.

[erozenfeld]

Under some jit stress modes the jit tries to inline StelemRef_Helper into
StelemRef. StelemRef is jitted very early and the jit is not yet prepared
to handle this inline.

Fixes #33298.

[Dotnet-GitSync-Bot]

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

[VSadov]

LGTM

[VSadov]

BTW. Do we have any way to indicate "do not inline, but tailcall is ok".

We have an assumption that NoInlining also implies NoTailcalling and that is inconvenient.

[jkotas]

Would it be better to reorder things to make this unnecessary?

[VSadov]

Reorder when the method is JIT-ted or reordering something in its implementation ?

[jkotas]

reorder when it is JITed, or when other things happen during startup

[erozenfeld]

We seem have an assumption that NoInlining also implies NoTailcalling and that is inconvenient.

We don't allow tail calls from callers marked NoInlining:

runtime/src/coreclr/src/vm/jitinterface.cpp

Lines 8270 to 8283 in dca3429

	if (!pCaller->IsNoMetadata())
	{
	// Do not tailcall from methods that are marked as noinline (people often use no-inline
	// to mean "I want to always see this method in stacktrace")
	DWORD dwImplFlags = 0;
	IfFailThrow(pCaller->GetMDImport()->GetMethodImplProps(callerToken, NULL, &dwImplFlags));
	if (IsMiNoInlining(dwImplFlags))
	{
	result = false;
	szFailReason = "Caller is marked as no inline";
	goto exit;
	}
	}

If the callee has NoInlining but the caller doesn't we can still do the tail call.

[jkotas]

Or at least add comment for the NoInlining attribute to describe why it is there.

[VSadov]

from the trace it looks like we would need to load CastCache type before jitting the helpers.

I think we do not need to initialize the casting cache, just load the type - since we would need the static field if we try to inline.

[VSadov]

If the callee has NoInlining but the caller doesn't we can still do the tail call.

I misread that line.

Would we still inline TryGet then?

[VSadov]

If I understand what Jan suggests; Is that just adding

MscorlibBinder::GetField(FIELD__CASTHELPERS__TABLE);

somewhere in PopulateManagedCastHelpers before we do DoPrestub ?

[VSadov]

Actually - which field is missing its type?
I kind of assumed it is the static for the table, but there are other types/fields involved.

[erozenfeld]

Would we still inline TryGet then?

Yes, having [NoInlining] on its caller won't affect the inlining decision for TryGet.
And I verified that we will tail call StelemRef_Helper from StelemRef.

[VSadov]

All is good then.

[erozenfeld]

Actually - which field is missing its type?
I kind of assumed it is the static for the table, but there are other types/fields involved.

It's s_table.

[VSadov]

Not sure about comment. There are 2 more methods in that file that use the same pattern - small NoInline helper that inlines TryGet + has some simple checks.

That is mostly to make sure that those checks do not end up inlined in the caller. The caller is typically the "fast path" and the no inline method is the next tier.

If you can come up with a simple explanation, please add to all 3 cases.

[erozenfeld]

I tried adding
MscorlibBinder::GetField(FIELD__CASTHELPERS__TABLE);
before
pMD = MscorlibBinder::GetMethod((BinderMethodID)(METHOD__CASTHELPERS__STELEMREF));
but got the same assert.

[jkotas]

I think it is ok as is.