JIT: Refactor around impDevirtualizeCall for GVM devirt

[hez2010]

Introduce a IsDevirtualizationCandidate instead of using IsVirtual for devirtualization candidate.

Contributes to #112596

[Copilot]

Copilot wasn't able to review any files in this pull request.

Files not reviewed (4)

• src/coreclr/jit/compiler.cpp: Language not supported
• src/coreclr/jit/gentree.h: Language not supported
• src/coreclr/jit/importercalls.cpp: Language not supported
• src/coreclr/jit/lower.cpp: Language not supported

[jakobbotsch]

Can you explain why this would be necessary if the whole thing is being implemented as an optimization that kicks in when the call address is CORINFO_HELP_VIRTUAL_FUNC_PTR?

[hez2010]

Can you explain why this would be necessary if the whole thing is being implemented as an optimization that kicks in when the call address is CORINFO_HELP_VIRTUAL_FUNC_PTR?

This would make it sharable with existing handling of virtual calls in impDevirtualizeCall (we have many call->IsVirtual() guards) and also make the code clearer as we can simply check whether a call is generic virtual by call->IsVirtualGeneric() instead of call->gtCallType == CT_INDIRECT && call->gtCallAddr->IsCall() && call->gtCallAddr->IsHelperCall(this, CORINFO_HELP_VIRTUAL_FUNC_PTR). If we add NAOT/R2R support, there will be more possible helpers we need to check. This check is also needed when we insert necessary InstParam for devirted gvm.
Alternatively we can put call->gtCallType == CT_INDIRECT && call->gtCallAddr->IsCall() && call->gtCallAddr->IsHelperCall(this, CORINFO_HELP_VIRTUAL_FUNC_PTR) under IsVirtual() and IsVirtualGeneric() without an additional flag.

[jakobbotsch]

This would make it sharable with existing handling of virtual calls in impDevirtualizeCall and also make the code clearer as we can simply check whether a call is generic virtual by call->IsVirtualGeneric() instead of call->gtCallType == CT_INDIRECT && call->gtCallAddr->IsCall() && call->gtCallAddr->IsHelperCall(this, CORINFO_HELP_VIRTUAL_FUNC_PTR). If we add NAOT/R2R support, there will be more possible helpers we need to check. This check is needed as we need to insert InstParam for devirted gvm when necessary.

impDevirtualizeCall should just be taught that if it can extract information from the call address, then it can use that for devirtualization. We do not want to have requirements like "nodes with GTF_CALL_VIRT_GENERIC must be CT_INDIRECT and have a certain shape of gtCallAddr". Requirements like that make the IR hard to work with (suddenly we can no longer change the operands of nodes freely).

[jakobbotsch]

Alternatively we can put call->gtCallType == CT_INDIRECT && call->gtCallAddr->IsCall() && call->gtCallAddr->IsHelperCall(this, CORINFO_HELP_VIRTUAL_FUNC_PTR) under IsVirtual() and IsVirtualGeneric() without an additional flag.

I do not think IsVirtual should be changed to care about this case of CT_INDIRECT. However, I agree that we can have some method like bool IsCallToVirtualGenericMethod(CORINFO_METHOD_HANDLE* handle, GenTree** runtimeLookup) that impDevirtualizeCall can use.

[hez2010]

We do not want to have requirements like "nodes with GTF_CALL_VIRT_GENERIC must be CT_INDIRECT and have a certain shape of gtCallAddr.

We pass the call node to impDevirtualizeCall just to be able to morph the call node after we done the devirtualization.
When devirtualize a method, impDevirtualizeCall only uses the method handle and context we passed to it explicitly. So things need to be sorted out before we call impDevirtualizeCall.
By introducing a IsVirtualGeneric() it falls into the IsVirtual() category, which enables the code path that calls impDevirtualizeCall. We put impDevirtualizeCall under a guard IsVirtual() in many places, like

runtime/src/coreclr/jit/importercalls.cpp

Lines 952 to 966 in 8a79637

	if (call->AsCall()->IsVirtual())
	{
	// only true object pointers can be virtual
	assert(call->AsCall()->gtArgs.HasThisPointer() &&
	call->AsCall()->gtArgs.GetThisArg()->GetNode()->TypeIs(TYP_REF));
	// See if we can devirtualize.
	constbool isExplicitTailCall = (tailCallFlags & PREFIX_TAILCALL_EXPLICIT) != 0;
	constbool isLateDevirtualization = false;
	impDevirtualizeCall(call->AsCall(), pResolvedToken, &callInfo->hMethod, &callInfo->methodFlags,
	&callInfo->contextHandle, &exactContextHnd, isLateDevirtualization, isExplicitTailCall,
	// Take care to pass raw IL offset here as the 'debug info' might be different for
	// inlinees.
	rawILOffset);

and we are also using IsVritual() to flag whether a virtual call has been actually devirted or not:

runtime/src/coreclr/jit/fginline.cpp

Lines 608 to 614 in 8a79637

	CORINFO_CONTEXT_HANDLE contextInput = context;
	context = nullptr;
	m_compiler->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &contextInput, &context,
	isLateDevirtualization, explicitTailCall);
	if (!call->IsVirtual())
	{

[jakobbotsch]

GenTreeCall::IsVirtual indicates something like: this call is virtual and needs special treatment for expansion throughout the JIT. For example, VSD calls come with special calling convention and the target of vtable calls is expanded based on the method handle very late.

Virtual generics do not have this flavor. They are modelled as normal indirect calls within the JIT that do not require any special treatment. Hence I do not think we should need to add a new category of calls in the JIT as part of this work. It does not look necessary to me. Instead, I would suggest that impDevirtualizeCall is refactored so that it can encompass this new behavior instead.

[hez2010]

How about introduce a new member function GenTreeCall::IsDevirtualizationCandidate?

[jakobbotsch]

How about introduce a new member function GenTreeCall::IsDevirtualizationCandidate?

That sounds good to me.

[AndyAyersMS]

I am reluctant to put a lot of time and energy into optimizing GVMs without strong motivation. It feels like we'd be better served trying to handle the CT_INDIRECT virtual cases which are likely far more common.

[hez2010]

I am reluctant to put a lot of time and energy into optimizing GVMs without strong motivation. It feels like we'd be better served trying to handle the CT_INDIRECT virtual cases which are likely far more common.

I do think there're at least some? See MihuBot/runtime-utils#1004
We are optimizing the GVM away for JsonNode, PLINQ, IQueryable, Logging and Hosting etc., where it would benefit frameworks like asp.net core and efcore.

[AndyAyersMS]

I am reluctant to put a lot of time and energy into optimizing GVMs without strong motivation. It feels like we'd be better served trying to handle the CT_INDIRECT virtual cases which are likely far more common.

I do think there're at least some? See MihuBot/runtime-utils#1004 We are optimizing the GVM away for JsonNode, PLINQ, IQueryable, Logging and Hosting etc., where it would benefit frameworks like asp.net core and efcore.

I'm not saying there is no benefit -- just that there are easier, less risky, and more impactful things we could do in this area.

[NinoFloris]

ADO.NET also has a few, specifically in the inner loop: DbDataReader.GetFieldValue and DbDataReader.GetFieldValueAsync

[hez2010]

Here is my pmi analysis: https://gist.github.com/hez2010/2ebacf9c663f34bb683d6f052aa1e5c3

Especially, every ILogger.Log is a GVM, which may also explain why we have a observable perf drop when logging is enabled even if we filtered all the logs to output nothing in asp.net core. And there're also plenty in Roslyn.

[hez2010]

@AndyAyersMS
This is a zero-diff PR that

1. Stop relying on IsVirtual to mark devirtualization candidates and introduce a helper method for it
2. Create a well-known argument for method handle in ldvirtftn so that we can find it later easily

[AndyAyersMS]

@hez2010 do you have time to resolve the conflicts here?

If not I can do it sometime soon.

[hez2010]

@hez2010 do you have time to resolve the conflicts here?

Done.

[hez2010]

No diffs as expected. PTAL @AndyAyersMS

[AndyAyersMS]

Thank you!