Stop spilling ldvirtftn

[hez2010]

We first need to stop spilling ldvirtftn before we can take the next step on GVM devirt.
Let's stop spilling them and see what we need to take to handle potential regressions.

Contributes to #112596

Fixes #121711

[Copilot]

Pull Request Overview

This PR refactors devirtualization handling by renaming a field and removing function pointer spilling for ldvirtftn operations. The change aims to improve performance by avoiding unnecessary temporary storage while preparing for further Generic Virtual Method (GVM) devirtualization improvements.

Key changes:

• Renamed wasArrayInterfaceDevirt to needsMethodContext to better reflect its broader purpose
• Removed spilling of ldvirtftn function pointers to temporary locals
• Added better handling for cloned "this" pointers in virtual function calls

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/coreclr/vm/jitinterface.cpp	Updated field assignments from wasArrayInterfaceDevirt to needsMethodContext
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp	Updated SuperPMI recording/replay to use the renamed field
src/coreclr/tools/superpmi/superpmi-shared/agnostic.h	Renamed struct field from wasArrayInterfaceDevirt to needsMethodContext
src/coreclr/jit/importercalls.cpp	Removed function pointer spilling, improved "this" pointer handling, updated field references
src/coreclr/jit/compiler.hpp	Fixed comment for helper function
src/coreclr/inc/corinfo.h	Updated struct definition and comments to use the new field name

[dotnet-policy-service]

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

[hez2010]

trigger mihubot since spmi jobs are broken @MihuBot

[EgorBo]

It looks like what you're trying to achieve is the devirtualization of indirect calls (GVM specifically), but you seems to be focused on a very specific case where the actual type is created right before the call (judging by your example in #112596 and the fact that you try to avoid spills).

I think we should focus on implementing #44610 for PGO first, e.g.

using System.Runtime.CompilerServices;

for (int i = 0; i < 200; i++)
{
    Test(new MyValueProcessor());
    Thread.Sleep(16);
}


[MethodImpl(MethodImplOptions.NoInlining)]
static void Test(IProcessor p)
{
    p.Process(42); // should devirtualize, but doesn't today.
}

public interface IProcessor
{
    void Process<T>(T item);
}

public class MyValueProcessor : IProcessor
{
    public void Process<T>(T item)
    {
        Console.WriteLine(item?.ToString());
    }
}

today we give up on instrumenting indirect calls.

And then, if you also want to have it divirtualized without the PGO (e.g. NAOT) you can just create fake (speculative) GDVs (which then will be folded once we hit a phase that can see through locals). Instead of just un-spilling the target that might be changed by the args?

cc @AndyAyersMS @dotnet/jit-contrib

[hez2010]

Test failure is #105124. Other than this CI is passing.
Change this PR to ready for review.

[MichalStrehovsky]

/azp run runtime-nativeaot-outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

I will take a look at the x86 asserts when I have some time.

[hez2010]

btw I guess we may want to stop spilling the call target for CORINFO_VIRTUALCALL_STUB, CORINFO_CALL_CODE_POINTER and impImportIndirectCall as well?

[jakobbotsch]

btw I guess we may want to stop spilling the call target for CORINFO_VIRTUALCALL_STUB, CORINFO_CALL_CODE_POINTER and impImportIndirectCall as well?

I think it will require its own evaluation of diffs and extending the optimization. I would leave it for a separate change.

[jakobbotsch]

/azp run jit-cfg

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @AndyAyersMS @EgorBo

This PR stops spilling the target for calls implemented via ldvirtftn in the importer, which is the way we implement GVMs. We were previously spilling the target with CHECK_SPILL_NONE, but this is incorrect because it induces a null check (see #121711). So this PR fixes that issue. Also, avoiding the spill gives us a simple uncontroversial way to devirtualize in some cases when we can extract information directly from gtCallAddr.

The problem with removing the spill is code size regressions. gtCallAddr is normally evaluated after arguments, and the backend is not good at handling the ABI constraints of the arguments when gtCallAddr contains calls. To counteract that we introduce a new optimize in lowering. The optimization tries to move the evaluation of the target to happen before the arguments. It effectively accomplishes what the previous CHECK_SPILL_NONE spill was doing, except with the right legality checks.

Diffs.
A mix of code size improvements/regressions. I think the regressions are small enough to take it as is. I have also left the optimization enabled in tier0 as it does not seem expensive and it helps LSRA significantly when it kicks in.

[AndyAyersMS]

Generally LGTM.

Are we able to CSE or hoist these lookups out of loops?

[jakobbotsch]

Are we able to CSE or hoist these lookups out of loops?

Doesn't look like it. We don't have an entry for CORINFO_HELP_VIRTUAL_FUNC_PTR in HelperCallProperties::init at all, so we think it mutates the heap. Which I guess it technically does since it fills various caches in VirtualDispatchHelpers.VirtalFunctionPointer. But I suppose it's the kind of heap mutation that we are ok with ignoring. I will submit a follow up about this.

[jakobbotsch]

Thanks!