Remove unnecessary runtime lookup for constrained callvirt

[trylek]

In my change adding support for default static virtual interface
method implementations I made a subtle bug that caused
behavioral change for some pre-existing constrained virtual calls
that newly started to require runtime lookup. This is unnecessary
and perf-negative, I have modified the code so that my change
kicks in only for static virtual methods.

Thanks

Tomas

/cc @dotnet/jit-contrib

[trylek]

Fixes: #73681

[trylek]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

LGTM. I've verified this returns the behavior of the example in #73606 back to what it was previously.

[jkotas]

Is R2R codegen correct for this case?

[AaronRobinsonMSFT]

@trylek Do we also need to update R2R as mentioned at #73823 (comment)?

[AaronRobinsonMSFT]

Perhaps in this area?

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

Lines 1736 to 1750 in 080f708

	//
	// Initialize callee context used for inlining and instantiation arguments
	//
	targetMethod = methodAfterConstraintResolution;
	if (targetMethod.HasInstantiation)
	{
	pResult->contextHandle = contextFromMethod(targetMethod);
	pResult->exactContextNeedsRuntimeLookup = targetMethod.IsSharedByGenericInstantiations;
	}
	else
	{
	pResult->contextHandle = contextFromType(exactType);
	pResult->exactContextNeedsRuntimeLookup = exactType.IsCanonicalSubtype(CanonicalFormKind.Any);

[markples]

lgtm for jitinterface logic (not familiar with r2r aspect)

[mangod9]

@trylek is this good to merge. Appears that needs to be ported to 7 too?

[AaronRobinsonMSFT]

@trylek is this good to merge. Appears that needs to be ported to 7 too?

We still need a response to the R2R query. I think that @jkotas is correct in that more needs to be updated.

[trylek]

I'm doing my best to produce an update this afternoon.

[trylek]

I have finally gotten to debugging the jitted and crossgenned version of the test described in the issue #73606. To answer @jkotas' question, I believe there's no counterpart logic in Crossgen2 as Crossgen2 still globally skips functions containing static virtual method calls using the following conditional statement:

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

Line 2066 in c5a0e55

if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_CALLVIRT) == 0 && pConstrainedResolvedToken != null)

The fix presented in this PR basically just scales back my getCallInfo change to only apply to SVMs, not normal constrained callvirt; as there's no equivalent logic in the Crossgen2 version of getCallInfo, I don't think there is anything to change. I have rebased the change against the latest main and I'm rerunning tests, at this point I'm not aware of any remaining blocking issues.

[trylek]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[trylek]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2922253202