JIT: Always use CallInfo method handle for devirtualization

[hez2010]

We already have the method handle in CallInfo, so we don't need to rely on the GenTree to extract the method handle for devirtualization.

Previously in #122023 we changed the JIT to only devirtualize a call when we can get the method handle from the GenTree, which was unnecessarily conservative especially when we already have the method handle in CallInfo.

Fixes #123391

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a devirtualization regression introduced in #122023 by changing how method handles are obtained during late devirtualization. Instead of extracting the method handle from the GenTree (which could fail for certain indirect virtual calls), the PR stores the method handle in LateDevirtualizationInfo during import and retrieves it from there during late devirtualization.

Changes:

• Added methodHnd field to LateDevirtualizationInfo structure to store the method handle
• Simplified IsDevirtualizationCandidate to just check if a call is virtual or generic virtual, removing the method handle extraction logic
• Updated late devirtualization to use the stored method handle instead of attempting to extract it from the tree

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/coreclr/jit/inline.h	Added methodHnd field to LateDevirtualizationInfo struct to store the method handle for late devirtualization
src/coreclr/jit/importercalls.cpp	Populated the new methodHnd field from callInfo->hMethod when creating late devirtualization info
src/coreclr/jit/gentree.h	Removed pMethHandle out parameter from IsDevirtualizationCandidate method signature
src/coreclr/jit/gentree.cpp	Simplified IsDevirtualizationCandidate to just return `IsVirtual()
src/coreclr/jit/fginline.cpp	Changed to retrieve method handle from gtLateDevirtualizationInfo->methodHnd instead of from out parameter, and removed now-unnecessary assertion

[EgorBo]

BenchmarkRunner.Run();

@hez2010 this is not a correct way, either remove it, or replace with BenchmarkSwitcher.FromAssembly(typeof(ContainsTrue).Assembly).Run(args);

[hez2010]

@EgorBot -amd -intel

using System.Runtime.CompilerServices;
using System.Text;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(ContainsTrue).Assembly).Run(args);

public class ContainsTrue
{
    private string[] _found;

    private string[] _array;
    private List<string> _list;

    [Params(512)]
    public int Size;

    [GlobalSetup(Targets = new[] { nameof(Array) })]
    public void SetupArray()
    {
        _found = ArrayOfUniqueValues(Size);
        _array = _found.ToArray();
    }

    [Benchmark]
    public bool Array()
    {
        bool result = default;
        string[] collection = _array;
        string[] found = _found;
        for (int i = 0; i < found.Length; i++)
            result ^= collection.Contains(found[i]);
        return result;
    }

    [GlobalSetup(Targets = new[] { nameof(ICollection) })]
    public void SetupList()
    {
        _found = ArrayOfUniqueValues(Size);
        _list = new List<string>(_found);
    }

    [Benchmark]
    public bool ICollection() => Contains(_list);

    [MethodImpl(MethodImplOptions.NoInlining)]
    private bool Contains(ICollection<string> collection)
    {
        bool result = default;
        string[] found = _found;
        for (int i = 0; i < found.Length; i++)
            result ^= collection.Contains(found[i]);
        return result;
    }

    private static string[] ArrayOfUniqueValues(int count)
    {
        string[] result = new string[count];

        var random = new Random(42);

        var uniqueValues = new HashSet<string>();

        while (uniqueValues.Count != count)
        {
            string value = GenerateRandomString(random, 1, 50);

            if (!uniqueValues.Contains(value))
                uniqueValues.Add(value);
        }

        uniqueValues.CopyTo(result);

        return result;
    }

    private static string GenerateRandomString(Random random, int minLength, int maxLength)
    {
        var length = random.Next(minLength, maxLength);

        var builder = new StringBuilder(length);
        for (int i = 0; i < length; i++)
        {
            var rangeSelector = random.Next(0, 3);

            if (rangeSelector == 0)
                builder.Append((char)random.Next('a', 'z'));
            else if (rangeSelector == 1)
                builder.Append((char)random.Next('A', 'Z'));
            else
                builder.Append((char)random.Next('0', '9'));
        }

        return builder.ToString();
    }
}

[hez2010]

Can confirm this fixes the regression seen in #123391

Method	Toolchain	Size	Mean	Error	Ratio
Array	Main	512	299.7 μs	2.99 μs	1.00
Array	PR	512	162.8 μs	3.12 μs	0.54
ICollection	Main	512	131.5 μs	2.50 μs	1.00
ICollection	PR	512	126.2 μs	0.90 μs	0.96

[hez2010]

Test failures seem unrelated.

[jakobbotsch]

Previously in #122023 we changed the JIT to only devirtualize a call when we can get the method handle from the GenTree

Wasn't this always the behavior? Before #122023 we used GetMethodHandle that also got it from the GenTree. So what exactly changed with #122023 that made this different?

[hez2010]

Previously in #122023 we changed the JIT to only devirtualize a call when we can get the method handle from the GenTree

Wasn't this always the behavior? Before #122023 we used GetMethodHandle that also got it from the GenTree. So what exactly changed with #122023 that made this different?

GetMethodHandle was only used for late devirtualization, in importer it only checked IsVirtual() || IsGenericVirtual() without extracting method handle from GenTree, and instead used the method handle from callInfo directly before #122023.

We have some virtual calls that are implemented as calli in the JIT despite they actually have a method handle. When importing those methods, we didn't actually use the CEE_CALLI code path but they were imported as calli in the end, which could lead the method handle to be thrown away after importer done. #122023 effectively blocked the devirtualization for them by accident, where we lost devirtualization opportunities for those methods in importer as well.

This PR unifies what we use for devirtualization, now we always have the method handle as long as the method is a virtual method.

[jakobbotsch]

Hmm, ok.
It is unfortunate that we are ending up with a redundant set of fields in LateDevirtualizationInfo that must be kept up to date with the actual source of truth in gtCallAddr.

[jakobbotsch]

Thanks.