Implement CEE_JMP for the interpreter#121130
Conversation
This makes around 20 more coreclr tests pass with the interpreter.
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for the CEE_JMP instruction in the CoreCLR interpreter, enabling approximately 20 additional coreclr tests to pass. The JMP instruction performs a tail call by transferring control to another method with the current method's arguments, without creating a new stack frame.
Key changes:
- Added INTOP_JMP opcode definition for interpreter operations
- Implemented CEE_JMP compilation in the interpreter compiler
- Added JMP execution logic that reuses the current frame for the target method
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/interpreter/inc/intops.def | Defines the new INTOP_JMP opcode with method handle operand |
| src/coreclr/interpreter/compiler.cpp | Implements compilation of CEE_JMP IL instruction to INTOP_JMP |
| src/coreclr/vm/interpexec.cpp | Adds execution logic for INTOP_JMP, including frame reinitialization and assertion for managed-only targets |
|
I would prefer to have written this on top of our tail-call logic (even though it would be slower), so as to avoid adding more complexity to the interpreter execution logic for a scenario which is vanishingly rare. |
|
There is still something not right, based on debugging some other test that accidentally also uses CEE_JMP. I am setting no-merge label until I sort it out. |
|
Problem fixed |
|
Can the JMP target be a generic method? If so, do we need to do something we aren't doing here? I don't see any logic that would handle it, but maybe it would just automatically work? |
|
@kg In principle, yes. And this implementation wouldn't work for that. I would be ok with making jmp to a generic method or static method on a generic type cause a BADCODE, since I don't think we have evidence that anyone has ever used jmp with a generics. Looking at the implementation of JMP in the jit, I'm not confident it handles that situation correctly either. This is another reason, I would prefer an implementation that used the general purpose tail-call logic we've built, as that is something we have real testing for. |
I think for cross-genericness jumps using generic context the JIT will badcode too here: runtime/src/coreclr/jit/importer.cpp Lines 7268 to 7274 in 7201a39 It might work if both the caller and callee use a generic context parameter. As an aside, I would echo what @davidwrighton suggests to represent this via tailcalls, if possible. In the JIT we have a similar explicit representation for Of course I don't know if the interpreter would be inclined to ever run into similar issues -- they have been mostly in optimized code. |
It is actually 55, not 20. The 20 was with an issue I had in the initial state of the PR and fixed later. |
|
Based on the comments from @davidwrighton and @jakobbotsch, I am working on moving the implementation to what @davidwrighton suggested. |
Would it make sense to add a test here that ensures both the JIT and Interpreter behave the same? |
|
I've pushed another commit that changes the CEE_JMP handling based on @davidwrighton and @jakobbotsch comments. It is now implemented purely at the compilation side. It loads the current method's args and then emulates the CEE_JMP using a tail call. |
AI generated these four test cases.⚙️ Test 1 — Jump from one regular method to another
```msil
.assembly JmpTest1 {}
.assembly extern mscorlib {}
.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
.method public static void Target() cil managed
{
.maxstack 8
ldstr "Target called"
call void [mscorlib]System.Console::WriteLine(string)
ret
}
.method public static void Source() cil managed
{
jmp void Program::Target()
}
.method public static void Main() cil managed
{
call void Program::Source()
ret
}
}
```
🟢 Expected behavior:
```
Target called
```
⚙️ Test 2 — Jump from generic method to generic method
```msil
.assembly JmpTest2 {}
.assembly extern mscorlib {}
.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
.method public static void Target<T>() cil managed
{
.maxstack 8
ldstr "Target<T> called"
call void [mscorlib]System.Console::WriteLine(string)
ret
}
.method public static void Source<T>() cil managed
{
jmp void Program::Target<!T>()
}
.method public static void Main() cil managed
{
call void Program::Source<int32>()
ret
}
}
```
🟢 Expected:
```
Target<T> called
```
⚙️ Test 3 — Jump from generic method → regular method
```msil
.assembly JmpTest3 {}
.assembly extern mscorlib {}
.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
.method public static void Target() cil managed
{
.maxstack 8
ldstr "Regular Target called"
call void [mscorlib]System.Console::WriteLine(string)
ret
}
.method public static void Source<T>() cil managed
{
jmp void Program::Target()
}
.method public static void Main() cil managed
{
call void Program::Source<int32>()
ret
}
}
```
🟢 Expected:
```
Regular Target called
```
⚙️ Test 4 — Jump from regular method → generic method
```msil
.assembly JmpTest4 {}
.assembly extern mscorlib {}
.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
.method public static void Target<T>() cil managed
{
.maxstack 8
ldstr "Generic Target<T> called"
call void [mscorlib]System.Console::WriteLine(string)
ret
}
.method public static void Source() cil managed
{
jmp void Program::Target<int32>()
}
.method public static void Main() cil managed
{
call void Program::Source()
ret
}
}
```
🟢 Expected:
```
Generic Target<T> called
``` |
Thank you @am11! I am going to try these tests and add them to this PR |
* Make the handling of generic methods with the CEE_JMP equivalent to JIT * Add tests verifying that
|
@MichalPetryka I've added the tests based on the code @am11 shared above and made the interpreter behave like the JIT. |
|
/ba-g The windows arm64 testing is down |
Apparently we got away without reporting this until the test in dotnet#121130 was added. Not reporting the calling convention results in this not kicking in: https://github.com/dotnet/runtime/blob/5e97723bdd8eb9eff95c52b7120cec6bfb5a0a19/src/coreclr/jit/importer.cpp#L7291-L7297 And failing the test.
Outerloops are broken. Apparently we got away without reporting this until the test in #121130 was added. Not reporting the calling convention results in this not kicking in: https://github.com/dotnet/runtime/blob/5e97723bdd8eb9eff95c52b7120cec6bfb5a0a19/src/coreclr/jit/importer.cpp#L7291-L7297 and failing the test. Cc @dotnet/ilc-contrib --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>
This makes around 20 more coreclr tests pass with the interpreter.