Fix tailcall regression with compiled F##41206
Conversation
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
@AndyAyersMS Is it possible to trigger the tailcall stress runs from the CI? |
|
Yes, it is one of the variations in runtime-coreclr jitstress. |
|
Nice! And a negative line diff. You can take |
|
@jkotas Once #41059 is merged, it would be good to rebase this one and run runtime-coreclr jitstress and runtime-coreclr libraries-jitstress on top of that change. #41059 forces all tail calls under tailcallstress to be helper-based calls. I'm just waiting for someone to review a test change in |
jkotas
force-pushed
the
issue-40864
branch
2 times, most recently
from
7c00593 to
3425dc3
Compare
|
@dotnet/jit-contrib This is blocked on #41059. Ready for review otherwise. |
jakobbotsch
left a comment
jakobbotsch
left a comment
There was a problem hiding this comment.
LGTM. Nice to see the optimizations also, that should remove a function call from the hot path.
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
Changes LGTM.
Should we also make updates to the design doc?
I have read the design doc and I believe everything in the doc is still accurate. I like that the design doc is focused on the concept and it does not go into explaining every detail. Let me know if there is a specific fact that you would like to see captured. |
This change skips instantiating stubs for direct tailcalls and instead passes the inst argument directly to the target method. Fixes dotnet#40864
| pCode->EmitCALL(METHOD__STUBHELPERS__NEXT_CALL_RETURN_ADDRESS, 0, 1); | ||
| // All arguments are loaded on the stack, it is safe to disable the GC reporting of ArgBuffer now. | ||
| // This is optimization to avoid extending argument lifetime unnecessarily. | ||
| // We still need to report the inst argument of shared generic code to prevent it from being unloaded. The inst |
There was a problem hiding this comment.
FYI: I have noticed that nothing guarantees that the tailcall target function pointer is kept alive and thus it may be unloaded. It would be a very corner-case situation that is unlikely to be ever hit in a real-world. I have opened #41314 on it.
|
@jkotas I recommend also running runtime-coreclr libraries-jitstress. |
|
The test failure is #40916. libraries-jitstress and jitstress optional runs passed. |
|
/backport to release/5.0 |
|
Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/223822150 |
ghost
locked as resolved and limited conversation to collaborators
5 participants
Fixes #40864