Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mono] Stop falling back to interpreter for runtime invoke #95469

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Nov 30, 2023

This PR stops most of the runtime invoke calls falling back to interpreter when MONO_AOT_MODE_LLVMONLY_INTERP mode is set. MONO_AOT_MODE_LLVMONLY_INTERP mode is set only when one of these two switches is passed to Mono AOT:

  • --llvmonly
  • --llvmonly-interp

WASM AOT is the only major target uses it. It was setup here:

<AOTMode Condition="'$(AOTMode)' == ''">LLVMOnlyInterp</AOTMode>

This PR should resolve some of, if not all of, the performance regression (dotnet/perf-autofiling-issues#24928) introduced by enabling WasmStripILAfterAOT by default.

This PR also contains a fix to the problem with Mono LLVM AOT when a runtime invoke calling a method with delegate parameters. The delegate parameter wasn't handled properly before.

The following C# program is a demonstration of the problem

public class Test
{
    unsafe public static void Main(string[] args)
    {
        void* fn = FunctionPointerMethods.GetFunctionPointer();
        MethodInfo m = typeof(FunctionPointerMethods).GetMethod(nameof(FunctionPointerMethods.CallFcnPtr_FP));
        m.Invoke(null, new object[] { (IntPtr)fn, 42 });  <--- Error out
    }
}

public static class FunctionPointerMethods
{
    public static bool CallMe(int i)
    {
        return i == 42;
    }

    public static unsafe bool CallFcnPtr_FP(delegate*<int, bool> fn, int value)
    {
        return fn(value);
    }

    public static unsafe delegate*<int, bool> GetFunctionPointer() => &CallMe;
}

cc: @lewing

@fanyang-mono
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono fanyang-mono changed the title [Mono][WIP] Try not fall back to interpreter for runtime invoke [Mono][WIP] Stop falling back to interpreter for runtime invoke Dec 7, 2023
@fanyang-mono fanyang-mono marked this pull request as ready for review December 7, 2023 19:29
@fanyang-mono fanyang-mono changed the title [Mono][WIP] Stop falling back to interpreter for runtime invoke [Mono] Stop falling back to interpreter for runtime invoke Dec 7, 2023
@fanyang-mono
Copy link
Member Author

All CI failures are known and unrelated to this PR.

@@ -3401,7 +3401,7 @@ mono_llvmonly_runtime_invoke (MonoMethod *method, RuntimeInvokeInfo *info, void
for (i = 0; i < sig->param_count; ++i) {
MonoType *t = sig->params [i];

if (!m_type_is_byref (t) && (MONO_TYPE_IS_REFERENCE (t) || t->type == MONO_TYPE_PTR)) {
if (!m_type_is_byref (t) && (MONO_TYPE_IS_REFERENCE (t) || t->type == MONO_TYPE_PTR || t->type == MONO_TYPE_FNPTR)) {
param_refs [i] = params [i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this also affect cases in iOS where interpreter fall back happens? @BrzVlad @vargaz

Copy link
Member Author

@fanyang-mono fanyang-mono Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally. Full aot, which iOS uses, didn't go through this codepath.

@SamMonoRT SamMonoRT requested a review from BrzVlad December 7, 2023 20:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants