Shared generics opcodes by davidwrighton · Pull Request #116990 · dotnet/runtime

davidwrighton · 2025-06-24T22:38:28Z

Implement the vast majority of the opcodes that are affected by shared generics

box, unbox, unbox.any, castclass, isinst, ldtoken, newarr, newobj of an mdarray

Add logic to enhance pretty printing of the Interp opcodes for these things. We can now see strings, and various magic pointer values get displayed in the disasm outputs.

Handle the 0 indirections generic lookup scenario

Many opcodes were able to use a set of generic helpercall opcodes. These opcodes have a naming convention of

a stands for addr of dvar or svar
g stands for do a generic lookup
p stands for pointer (acquired from either an svar or a dataitem)
v stands for a void return.

Its not fully descriptive, but naming is hard, and this works.

- boxint, unbox, unbox.any, cast, isinst, ldtoken, newarr, newobj Added additional diagnostic printing capabilities, to print strings, and method/class names in interesting scenarios Known continuing work - MDArray new - static variables and class constructor triggering

…_opcodes_fillout

Copilot

Pull Request Overview

Implements shared generic support in the interpreter by adding new opcodes, wiring up generic lookups, and enhancing disassembly printing for generic contexts.
Key changes:

Added shared‐generics helper opcodes (call.helper.pg, newobj.generic, etc.) and their handling in interpexec.cpp and intops.def/h.
Extended the compiler to emit and print generic lookup data (InterpGenericLookup) and pointer tagging for improved pretty‐printing.
Expanded the JIT/interpreter tests in Interpreter.cs to cover shared generics scenarios (box/unbox, castclass, newarr, new MD array, isinst, ldtoken, newobj).

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tests/JIT/interpreter/Interpreter.cs	Added tests for most shared‐generics opcodes
src/coreclr/vm/interpexec.cpp	Implemented runtime generic lookups via `DO_GENERIC_LOOKUP` macro
src/coreclr/interpreter/intops.h	Declared new opcode argument types for generic helpers
src/coreclr/interpreter/intops.def	Defined new helper‐ and lookup‐based opcodes and replaced old ones
src/coreclr/interpreter/datastructs.h	Marked `GetSize()` and `GetUnderlyingArray()` as `const`
src/coreclr/interpreter/compiler.h	Introduced `InterpGenericLookup` metadata and new emit helpers
src/coreclr/interpreter/compiler.cpp	Wired up emission of generic helper calls and enhanced printing

Comments suppressed due to low confidence (1)

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

The new shared-generics tests cover most opcodes, but there’s no dedicated test for ldtoken on a generic type. Consider adding a TestSharedGenerics_LdToken<T>() to verify interpreter lookup and printing for token handles.

        return true;

Copilot · 2025-06-24T22:41:04Z

src/coreclr/interpreter/compiler.h

+#ifdef DEBUG
+        void *ptr = (void*)clsHnd;
+        if (!PointerInNameMap(ptr))
+        {
+            AddPointerToNameMap(ptr, PointerIsClassHandle);
+        }
+#endif DEBUG
+    }
+
+    void DeclarePointerIsMethod(CORINFO_METHOD_HANDLE methodHnd)
+    {
+#ifdef DEBUG
+        void *ptr = (void*)methodHnd;
+        if (!PointerInNameMap(ptr))
+        {
+            AddPointerToNameMap(ptr, PointerIsMethodHandle);
+        }
+#endif DEBUG
+    }
+
+    void DeclarePointerIsString(void* stringLiteral)
+    {
+#ifdef DEBUG


The debug guards use #ifdef DEBUG but the project typically defines _DEBUG. Consider changing #ifdef DEBUG to #ifdef _DEBUG so these debug blocks actually compile in debug builds.

Suggested change

#ifdef DEBUG

void *ptr = (void*)clsHnd;

if (!PointerInNameMap(ptr))

{

AddPointerToNameMap(ptr, PointerIsClassHandle);

}

#endif DEBUG

}

void DeclarePointerIsMethod(CORINFO_METHOD_HANDLE methodHnd)

{

#ifdef DEBUG

void *ptr = (void*)methodHnd;

if (!PointerInNameMap(ptr))

{

AddPointerToNameMap(ptr, PointerIsMethodHandle);

}

#endif DEBUG

}

void DeclarePointerIsString(void* stringLiteral)

{

#ifdef DEBUG

#ifdef _DEBUG

void *ptr = (void*)clsHnd;

if (!PointerInNameMap(ptr))

{

AddPointerToNameMap(ptr, PointerIsClassHandle);

}

#endif // _DEBUG

}

void DeclarePointerIsMethod(CORINFO_METHOD_HANDLE methodHnd)

{

#ifdef _DEBUG

void *ptr = (void*)methodHnd;

if (!PointerInNameMap(ptr))

{

AddPointerToNameMap(ptr, PointerIsMethodHandle);

}

#endif // _DEBUG

}

void DeclarePointerIsString(void* stringLiteral)

{

#ifdef _DEBUG

Copilot · 2025-06-24T22:41:05Z

src/coreclr/interpreter/compiler.cpp

+                    m_pStackPointer--;
+                    EmitLdLocA(m_pStackPointer[0].var);
+                }
+                m_ip += 5;


Missing break; at the end of the case CEE_UNBOX: block causes fall-through into case CEE_UNBOX_ANY: and will execute unintended logic. Add a break; after this line.

Suggested change

m_ip += 5;

m_ip += 5;

break;

Copilot · 2025-06-24T22:41:05Z

src/coreclr/vm/interpexec.cpp

    }
 }

+#define DO_GENERIC_LOOKUP(genericVar, lookupIndex) \


[nitpick] The DO_GENERIC_LOOKUP macro is very large and complex, leading to duplicated logic and reduced readability. Consider refactoring this into a dedicated function or a set of smaller helpers.

I agree with copilot that I'd love to see this outlined into function(s). As-is the line number information when debugging it is gonna be kinda miserable.

I agree, I was debugging an issue with it in the past and I had to expand the macro manually to be able to figure out what's wrong.

K, I've cleaned that up.

kg · 2025-06-24T23:23:05Z

src/coreclr/interpreter/compiler.h

+    TArray<PointerToName> m_pointerToNameMap;
+    bool PointerInNameMap(void* ptr)
+    {
+        for (int32_t i = 0; i < m_pointerToNameMap.GetSize(); i++)


Is this linear search going to become a problem for us in large method bodies? Do we need to be using a hashtable yet? We have simdhash available since it's used in stackmap.cpp, though it doesn't have a predefined variant that would hold PointerToName (it's easy to add one).

I've started to use dn_simd_hash_ptr_ptr which is close enough. I added a C++ wrapper to handle lifetime.

fyi if you look at dn-simdhash-ght-compatible.c you can see how to define custom callbacks and whatnot. but there's nothing wrong with using ptr_ptr if you're okay with an extra allocation per item.

kg · 2025-06-24T23:26:28Z

src/coreclr/interpreter/compiler.h

+        GenericHandleData() = default;
+
+        HelperArgType argType = HelperArgType::Value;
+        int genericVar = -1; // Set to a meaningful value if HelperArgType is GenericResolution


Something more specific than 'a meaningful value' would be helpful here since by browsing around the vicinity I can't tell what makes the value meaningful or how I would distinguish between a valid genericVar and an invalid one. It may become obvious to me as I continue reading the PR, though!

My assumption upon first glance is it's the zero-based index of the generic parameter in the method's set of reachable generic parameters (where i use 'reachable generic parameters' to mean the set of all the enclosing type parameters + all the method parameters)

OK, I'm at this point again and it's 'the interp var containing the generic dictionary' I think?

The interp var containing the generic context argument. (The dictionary is a set of indirections away typically)

kg · 2025-06-24T23:31:18Z

src/coreclr/interpreter/compiler.cpp

+        m_pLastNewIns->SetSVar(arg2);
+        m_pLastNewIns->SetDVar(resultVar);
+
+        m_pLastNewIns->data[1] = handleData.dataItemIndex;


[nitpick] There's an asymmetry here where in the GenericResolution arm of these ifs, we set data[0] and data[1] right next to each other, but in the other arm we set them separately. It seems like it could hide mistakes. It would be cool if both sides were the same shape so the differences were clearer to the reader.

I've made this cleaner.

src/coreclr/interpreter/compiler.cpp

kg · 2025-06-24T23:41:28Z

src/coreclr/interpreter/compiler.cpp

+    if (embedInfo.lookup.lookupKind.needsRuntimeLookup)
+    {
+        CORINFO_RUNTIME_LOOKUP_KIND runtimeLookupKind = embedInfo.lookup.lookupKind.runtimeLookupKind;
+        InterpGenericLookup *pRuntimeLookup = (InterpGenericLookup*)AllocMethodData(sizeof(InterpGenericLookup));


Is it the case that GenericHandleToGenericHandleData could get called for the same embedInfo multiple times, and we'd bloat the dataitems table with lots of duplicate lookups? i.e. if a method has two generic parameters T and U, are we going to end up with a unique dataitem for every use of T and every use of U? Or just two data items, one for T and one for U, that get reused through some other mechanism I haven't noticed yet?

Yeah, this is a problem. I've added a general purpose hashing mechanism for embedding data into the dataitems array, and swapped over to using that hash even for regular pointers. This should address any reasonable scalability issues here.

src/coreclr/interpreter/compiler.cpp

kg · 2025-06-25T00:25:23Z

src/coreclr/vm/interpexec.cpp

+                            size_t lastOffset = pLookup->offsets[pLookup->indirections - 1];                \
+                            if (pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK)                               \
+                            {                                                                               \
+                                /* Last indirection is the size*/                                           \


This is news to me. I don't see corresponding information in interpretershared.h.

kg · 2025-06-25T00:25:41Z

src/coreclr/vm/interpexec.cpp

+                    if (pLookup->indirections != 0)                                                         \
+                    {                                                                                       \
+                        lookup = *(uint8_t**)(lookup + pLookup->offsets[0]);                                \
+                        if (pLookup->indirections >= 3)                                                     \


This and the 4 look like they're off by 1, but I assume that's on purpose and goes with the 'last indirection is the size' below

kg · 2025-06-25T00:28:39Z

src/coreclr/vm/interpexec.cpp

+                        pMD = LOCAL_VAR(genericVar, MethodDesc*);                                           \
+                        lookup = (uint8_t*)pMD;                                                             \
+                    }                                                                                       \
+                    void* result = 0;                                                                       \


[nitpick] It'd be nice if this name made it a little clearer that it's from the macro, or if it was defined at the top of the macro instead of hiding in the middle

kg · 2025-06-25T00:31:18Z

src/coreclr/vm/interpexec.cpp

    }
 }

+#define DO_GENERIC_LOOKUP(genericVar, lookupIndex) \


[nitpick] It's weird that genericVar is usually just ip[x] and then lookupIndex is y. It might make sense for genericVar to also just be x so there's consistency.

kg · 2025-06-25T00:36:09Z

LGTM other than the missing break copilot noticed.

dotnet-policy-service · 2025-06-25T04:10:38Z

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

…kups, and embed the InterpGenericLookup in the dataitems array directly Fix generic lookups to handle the usehelper case Shrink the InterpGenericLookup to be the actual size needed, using smaller data fields when possible Improve comments on the interpreter around what the sizeOffset means

src/native/containers/dn-simdhash-string-ptr.h

Co-authored-by: Adeel Mujahid <[email protected]>

kg · 2025-06-26T23:54:57Z

src/coreclr/interpreter/compiler.cpp

+        dst->sizeOffset = 0;
+        dst->offsets[0] = 0;
+        dst->offsets[1] = 0;
+        dst->offsets[2] = 0;
+        dst->offsets[3] = 0;
+    }
+    else
+    {
+        dst->sizeOffset = src->sizeOffset;
+        dst->offsets[0] = (uint16_t)GetGenericLookupOffset(src, 0);
+        dst->offsets[1] = (uint16_t)GetGenericLookupOffset(src, 1);
+        dst->offsets[2] = (uint16_t)GetGenericLookupOffset(src, 2);
+        dst->offsets[3] = (uint16_t)GetGenericLookupOffset(src, 3);
+    }


A loop would be nice but not mandatory

src/coreclr/interpreter/compiler.cpp

kg · 2025-06-27T00:23:42Z

src/coreclr/interpreter/compiler.h

+
+        size_t sizeOfStruct = sizeof(InterpGenericLookup);
+
+        assert(sizeOfFieldsConcatenated == sizeOfStruct); // Assert that there is no padding in the struct, so a fixed size hash unaware of padding is safe to use


This can be a static_assert right?

Yes, but its easier to see what's wrong when its a runtime error, and since we have a functional PR system, it will be caught pretty quickly.

But there is a good point that that doesn't really happen in release builds.

kg · 2025-06-27T00:25:45Z

src/coreclr/interpreter/compiler.h

+    bool PointerInNameMap(void* ptr)
+    {
+        void* result;
+        if (dn_simdhash_ptr_ptr_try_get_value(m_pointerToNameMap.GetValue(), ptr, &result))
+        {
+            return true;
+        }
+        return false;
+    }
+    void AddPointerToNameMap(void* ptr, const char* name)
+    {
+        if (!PointerInNameMap(ptr))
+        {
+            dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name);
+        }
+    }


As things are written right now, we don't need PointerInNameMap at all, and the try_add operation will harmlessly return 0 if the key ptr already exists.

kg · 2025-06-27T00:28:43Z

src/coreclr/interpreter/compiler.h

+    bool PointerInNameMap(void* ptr)
+    {
+        void* result;
+        if (dn_simdhash_ptr_ptr_try_get_value(m_pointerToNameMap.GetValue(), ptr, &result))


fyi, you can pass NULL instead of &result and simdhash try_get_value will do what you want (not copy, just return whether a value was found or not)

kg · 2025-06-27T00:30:32Z

src/coreclr/interpreter/compiler.h

+    assert(sizeof(VarSizedData) == sizeof(void*));
+    assert(sizeof(T) % sizeof(void*) == 0);
+    assert(sizeof(VarSizedDataWithPayload<T>) == sizeof(T) + sizeof(void*));


These should probably also be static_assert instead of assert, so that they will be checked in release builds

BrzVlad · 2025-06-27T11:06:03Z

src/coreclr/vm/interpexec.cpp

+                    int base = 4;
+                    DO_GENERIC_LOOKUP(ip[2], base + 1);
+
+                    HELPER_FTN_PP helperFtn = GetPossiblyIndirectHelper<HELPER_FTN_PP>(pMethod->pDataItems[ip[base]]);


There is confusing casting between HELPER_FTN_PP and HELPER_FTN_PP_2. Also it seems we are going with the convention _{retType}_{argType}{argType}... So HELPER_FTN_PP_2 should actually be renamed to HELPER_FTN_P_PP. HELPER_FTN_PP should be HELPER_FTN_P_P. Otherwise it is just a mess.

Mmm... fair. I was just copying/pasting here, and mostly spending my time thinking about the compiler side. I'm going to eliminate the code sharing between opcodes here, as I think its obfuscating the logic, and simplify this out.

BrzVlad · 2025-06-27T11:06:19Z

src/coreclr/vm/interpexec.cpp

+
+                    HELPER_FTN_PP helperFtn = GetPossiblyIndirectHelper<HELPER_FTN_PP>(pMethod->pDataItems[ip[base]]);
+                    void* helperArg = pMethod->pDataItems[ip[base + 1]];
+                    LOCAL_VAR(ip[1], void*) = ((HELPER_FTN_PP_2)helperFtn)(helperArg, LOCAL_VAR_ADDR(ip[2], void*));


here as well

BrzVlad · 2025-06-27T11:08:23Z

src/coreclr/vm/interpexec.cpp

+                case INTOP_CALL_HELPER_V_AGP:
+                {
+                    int base = 4;
+                    DO_GENERIC_LOOKUP(ip[2], base + 1);


Seems cleaner for DO_GENERIC_LOOKUP to receive the data item index directly rather than an offset into the currently executing instruction.

BrzVlad · 2025-06-27T11:16:11Z

src/coreclr/vm/interpexec.cpp

+    return result;
+}
+
+#define DO_GENERIC_LOOKUP(genericVar, lookupIndex) \


Do we even need this define anymore ? Doesn't seem to add more clarity. We could just replace it with a method returning result explicitly

- use static_asserts where possible - remove the mess of casts and temp variables around helper function opcodes - Remove the DO_GENERIC_LOOKUP macro since it wasn't doing much of use anymore - Use the simdhash apis a little better

davidwrighton added 5 commits June 24, 2025 14:30

Add multidimensional array handling

944196a

Merge branch 'main' of github.com:dotnet/runtime into shared_generics…

fb249e5

…_opcodes_fillout

Handle the generic shared 0 indirections scenario

2e674d2

Make helper call opcodes have some semblance of a naming convention

b4bdb44

Copilot AI review requested due to automatic review settings June 24, 2025 22:38

davidwrighton requested review from BrzVlad, janvorli and kg as code owners June 24, 2025 22:38

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 24, 2025

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

Copilot AI reviewed Jun 24, 2025

View reviewed changes

kg reviewed Jun 24, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

kg reviewed Jun 24, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

kg reviewed Jun 24, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

kg reviewed Jun 24, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

kg reviewed Jun 25, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

kg reviewed Jun 25, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

kg reviewed Jun 25, 2025

View reviewed changes

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

browser-wasm Windows build error #116746

Closed

filipnavara added area-CodeGen-Interpreter-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 25, 2025

davidwrighton added 3 commits June 25, 2025 13:31

First round of PR feedback

7753ae6

Fix unix build breaks

562fb5e

This was referenced Jun 26, 2025

[iOS/tvOS] System.Runtime.Tests crash with signal 4 #116815

Closed

wasm build failure in CI #117017

Open

Fix build issues

119e65b

am11 reviewed Jun 26, 2025

View reviewed changes

src/native/containers/dn-simdhash-string-ptr.h Show resolved Hide resolved

Update src/native/containers/dn-simdhash-string-ptr.h

2ccdbbe

Co-authored-by: Adeel Mujahid <[email protected]>

This was referenced Jun 26, 2025

CI flakiness: mono interpreter build getting killed #114123

Open

Assertion failed 'm_dfsTree->Contains(dominator) && m_dfsTree->Contains(dominated)' #117039

Closed

kg reviewed Jun 26, 2025

View reviewed changes

kg reviewed Jun 27, 2025

View reviewed changes

src/coreclr/interpreter/compiler.cpp Show resolved Hide resolved

davidwrighton enabled auto-merge (squash) June 27, 2025 00:15

kg reviewed Jun 27, 2025

View reviewed changes

kg approved these changes Jun 27, 2025

View reviewed changes

davidwrighton merged commit 50be35a into dotnet:main Jun 27, 2025
145 of 156 checks passed

BrzVlad reviewed Jun 27, 2025

View reviewed changes

github-actions bot locked and limited conversation to collaborators Jul 28, 2025


		size_t sizeOfStruct = sizeof(InterpGenericLookup);

		assert(sizeOfFieldsConcatenated == sizeOfStruct); // Assert that there is no padding in the struct, so a fixed size hash unaware of padding is safe to use

Conversation

davidwrighton commented Jun 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 24, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

kg commented Jun 25, 2025

Uh oh!

dotnet-policy-service bot commented Jun 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!