Interpreter shared generics support #116357

davidwrighton · 2025-06-06T00:16:20Z

Add support for shared generics to the interpreter

Handle shared generics calling convention details for the newobj, and call opcodes
Swap over to using getCallInfo on the jit interface to get information about how a call should be handled
Add new instructions to resolve a generic dictionary lookup (generic.method, generic.class, generic.this)
Add new instructions ldtoken.var and newobj.var which can use the result of a generic dictionary lookup. These are variants on ldtoken and newobj instructions.
- we will eventually need variants for newarr, box, unbox.any, unbox, call.helper.pp and call.helper.pp2
Rework how call opcodes work so that it uses GetMultiCallableAddrOfCode for dispatch instead of the NativeCode on a method, and stop re-using the NativeCode field for interpreter things, and instead add a new field to MethodDesc for that. This made it simple to enable support for string constructors.
Also some drive by improvements to improve diagnostics. Notably startIp now points at a strongly typed struct which allows much easier examination of the interpreter stack using the debugger.

This PR needs a bit of work, notably the FillTempVarWithToken api used to implement the shared generic lookups is a really bad api, but now that its built and we have some uses, I'd like to refactor into something more useable. There are also incomplete features like full static virtual dispatch support and gvms, but this change is quite large enough already.

1. Handle calling convention for CEE_CALLing 2. Use getCallInfo for handling the call target resolution 3. Add handling for the param type arg as well as an ability to use the generic dictionary 4. Add a new ldtoken interpreter instruction which can take the ldtoken input as a var instead of as a data item - Not done - Anything to do with newobj - Anything to do with calli or the CODEPOINTER path - Anything to do with the LDVIRTFTN path

Copilot

Pull Request Overview

Add support for shared generics in the interpreter by extending call dispatch, adding new lookup instructions, and updating metadata & tests.

Handle shared generics calling conventions in newobj/call, add generic.* and .var instructions
Store interpreter bytecode pointer in MethodDesc and switch dispatch to GetMultiCallableAddrOfCode
Extend tests (TestStringCtor, TestSharedGenerics) and use a typed startIp for better diagnostics

Reviewed Changes

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

Show a summary per file

File	Description
src/tests/JIT/interpreter/Interpreter.cs	Added verbose `Console.WriteLine` before each test and new shared‐generics tests
src/coreclr/vm/prestub.cpp	Store interpreter code pointer into `MethodDesc` and update frame `startIp`
src/coreclr/vm/method.hpp	Added `m_interpreterCode` field and accessors for interpreter bytecode
src/coreclr/vm/jitinterface.cpp	Added `CORINFO_CALLINFO_DISALLOW_STUB` branch in `getCallInfo`
src/coreclr/vm/jithelpers.cpp	Split out `GenericHandleWorkerCore` and wrapped with QCall
src/coreclr/vm/interpexec.h & .cpp	Replaced raw `int32_t*` with `PTR_InterpByteCodeStart`, updated dispatch to bytecode
src/coreclr/vm/callstubgenerator.h & .cpp	Changed `ProcessArgument` to take pointer, handle string ctor convention
src/coreclr/vm/amd64/asmconstants.h	Adjusted offset for `InstantiatedMethodDesc::m_pPerInstInfo`
src/coreclr/inc/corinfo.h	Defined new `CORINFO_CALLINFO_DISALLOW_STUB` flag
src/coreclr/interpreter/intops.h & .def	Introduced `InterpOpGenericLookup`, `InterpOpLdPtr`, new lookup opcodes
src/coreclr/interpreter/interpretershared.h	Added `InterpByteCodeStart` struct to hold method pointer + bytecode
src/coreclr/interpreter/compiler.h	Updated `EmitCall` signature to support shared‐generics

Comments suppressed due to low confidence (1)

src/tests/JIT/interpreter/Interpreter.cs:386

[nitpick] The repeated Console.WriteLine and Environment.FailFast pattern clutters the test runner; consider refactoring into a helper method or iterating over a list of test names to reduce duplication.

        Console.WriteLine("Sum");

Copilot · 2025-06-06T00:17:54Z

src/coreclr/interpreter/compiler.h

@@ -458,7 +463,7 @@ class InterpCompiler
    void    EmitUnaryArithmeticOp(int32_t opBase);
    void    EmitShiftOp(int32_t opBase);
    void    EmitCompareOp(int32_t opBase);
-    void    EmitCall(CORINFO_CLASS_HANDLE constrainedClass, bool readonly, bool tailcall);
+    void    EmitCall(CORINFO_RESOLVED_TOKEN* constrainedClass, bool readonly, bool tailcall, bool newObj);


The signature of EmitCall was updated in the header but its implementation in compiler.cpp has not been adjusted; this mismatch will cause a build failure. Update the definition in the .cpp file to match the new parameters.

janvorli · 2025-06-06T19:36:56Z

src/coreclr/interpreter/compiler.h

+
+    enum class GenericHandleEmbedOptions
+    {
+        support_use_as_flags = -1,


A nit - it would be nice to use consistent naming convention.

This is a magic value that makes it possible (in conjunction with enum_class_flags.h to use | and & and the HasFlag helper method with an enum class. I'll add a comment.

Ah, I didn't know we had that magic stuff :-)

janvorli · 2025-06-06T19:38:24Z

src/coreclr/interpreter/interpretershared.h

+#else
+    DPTR(InterpMethod) const InterpMethod; // Pointer to the InterpMethod structure
+#endif
+    const int* GetByteCodes() const


The interpreter loop uses explicit int32_t, it would be nice to be consistent with it here too.

janvorli · 2025-06-06T19:42:52Z

src/coreclr/vm/interpexec.cpp

@@ -130,7 +144,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
    }

    int32_t returnOffset, callArgsOffset, methodSlot;
-    const int32_t *targetIp;
+    PTR_InterpByteCodeStart targetIp;


Why do we need the PTR_ here when this is never compiled for DAC?

We do not. The regular * would be good enough for this non-dac code. I added the concept of PTR_InterpByteCodeStart as that will at some point be needed by the DAC to understand the call stack, etc.

Having the PTR_InterpByteCodeStart at other places is fine and useful. I would prefer plain pointer in code that's not DAC related if you don't mind.

janvorli · 2025-06-06T20:10:36Z

src/coreclr/interpreter/compiler.cpp


    offset = 0;

    INTERP_DUMP("\nCreate IL Vars:\n");

    CORINFO_ARG_LIST_HANDLE sigArg = m_methodInfo->args.args;
-    for (int i = 0; i < numArgs; i++) {
+    for (int i = 0; i < (numArgs + hasParamArg); i++) {


I'd like to understand why we need to have the paramArg variable index to be somewhere else (after the IL locals in our case), but offset-wise, it is still located at the first or second slot of the arguments. Why do we care about the actual index of the variable not being in the range of indices of the other arguments?

I did that so that the handling of ldarg and ldloc could remain with its current logic (where ldarg variable indices exactly match that of the IL args, and ldloc indices are offset from zero by exactly the count number of IL args. This could of course be finagled around, but I thought this was the simplest solution. If you'd prefer to have a more complex mapping for those, I can do that too.

That makes sense, I am fine with it. I just wanted to understand "why".

It certainly deserves a comment, since it's fairly surprising.

janvorli

LGTM, thank you!

janvorli · 2025-06-06T21:32:00Z

src/coreclr/vm/interpexec.cpp

@@ -130,7 +144,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr
    }

    int32_t returnOffset, callArgsOffset, methodSlot;
-    const int32_t *targetIp;
+    PTR_InterpByteCodeStart targetIp;


Having the PTR_InterpByteCodeStart at other places is fine and useful. I would prefer plain pointer in code that's not DAC related if you don't mind.

jkotas · 2025-06-06T23:43:14Z

src/coreclr/inc/corinfo.h

@@ -1358,7 +1358,7 @@ enum CORINFO_CALLINFO_FLAGS
    CORINFO_CALLINFO_ALLOWINSTPARAM = 0x0001,   // Can the compiler generate code to pass an instantiation parameters? Simple compilers should not use this flag
    CORINFO_CALLINFO_CALLVIRT       = 0x0002,   // Is it a virtual call?
    // UNUSED                       = 0x0004,
-    // UNUSED                       = 0x0008,
+    CORINFO_CALLINFO_DISALLOW_STUB  = 0x0008,   // Do not use a stub for this call, even if it is a virtual call.


Nit: Add this to the managed view as well.

davidwrighton added 4 commits June 3, 2025 15:27

Implement newobj for normal classes and structs

1b76ff1

Merge branch 'main' of https://github.com/dotnet/runtime

b47739f

Pass all the tests, and add some more

b5f3cbc

Copilot AI review requested due to automatic review settings June 6, 2025 00:16

davidwrighton requested review from BrzVlad, janvorli and kg as code owners June 6, 2025 00:16

github-actions bot added the area-VM-coreclr label Jun 6, 2025

dotnet-policy-service bot assigned davidwrighton Jun 6, 2025

Copilot AI reviewed Jun 6, 2025

View reviewed changes

filipnavara added area-CodeGen-Interpreter-coreclr and removed area-VM-coreclr labels Jun 6, 2025

davidwrighton added 2 commits June 6, 2025 09:53

Fix build breaks due to extra field in MethodDesc

3016e59

Fix factoring and naming of generic lookups

21b0b47

janvorli reviewed Jun 6, 2025

View reviewed changes

Attempt to fix build issues and code review comments

d475b98

janvorli approved these changes Jun 6, 2025

View reviewed changes

Improve factoring

017362b

This was referenced Jun 6, 2025

CoreCLR Interpreter - Shared generics support #116395

Open

CoreCLR Interpreter #112158

Open

jkotas reviewed Jun 6, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jun 7, 2025

CI flakiness: mono interpreter build getting killed #114123

Open

Interpreter shared generics support #116357

Are you sure you want to change the base?

Interpreter shared generics support #116357

Conversation

davidwrighton commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidwrighton commented Jun 6, 2025 •

edited

Loading