[interp] Remove return value indirection #37004
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When returning from a method we used to save the return value into frame->retval. We would then have separate opcodes for normal and void call and, depending on the type of call, we would access the retval and push it on the stack. In case of valuetype returns, the value is not returned in the proper space so we have an additional MINT_VTRESULT that copies the return around in the caller frame. In addition to this we had retval, is_void locals, saved as part of the current frame state which just make things confusing and are not needed. This commit simplifies the call procedure by completely moving the responsability of the return to the called method. Simply put, once a method is ready for the call, it will leave its stack and vtstack to the state after pop-ing the arguments. The called method will then directly push the result to the parent frame's stack/vtstack. This means the calling code doesn't care about the type of the return anymore, the IL instructions after the call will just expect the return on the stack, if any, and use it naturally. We still make use of frame->retval for interp entry frames (since we don't have a parent frame to push to), for pinvoke and jit_call frames out of convenience, but we should consider removing this variable to further trim down InterpFrame.
MINT_CALLVIRT is soon to be removed while MINT_CALL is the most common call opcode.
The callee should push the vt stack if there is a vt return, except when we are calling out of the interpreter, in which case the calling opcodes handle the push explicitly. We weren't handling the case of exiting the interpreter when doing a jit call through MINT_CALLVIRT. In order to keep the same structure for MINT_CALLVIRT, which doesn't need to know about the vt_res_size, we handle the vtstack change when doing a jit call by caching the return size in the InterpMethod, together with other jit_call information. Maybe we can refactor this later.
Add the opcode and set its data in the same place, making the code easier to follow.
This opcode had two different cases embedded in it. Clean up the code, split the opcode in two, removing also a dirty recursive call.
There are over a hundred tests calling GC.WaitForPendingFinalizers and potentially flaky with mono. We might have to disable all of them or tweak them to be mono friendly.
Member
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
vargaz
reviewed
May 27, 2020
|
|
||
| frame->ip = ip; | ||
| #ifdef ENABLE_EXPERIMENT_TIERED | ||
| ip += 5; |
Contributor
There was a problem hiding this comment.
This is not good anymore, not sure whenever it matters.
Member
Author
There was a problem hiding this comment.
Yeah, MINT_JIT_CALL2 still expects the old calling convention. Will fix it some other time to not further complicate this PR.
Contributor
|
Looks nice. |
vargaz
approved these changes
May 27, 2020
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When returning from a method we used to save the return value into frame->retval. We would then have separate opcodes for normal and void call and, depending on the type of call, we would access the retval and push it on the stack. In case of valuetype returns, the value is not returned in the proper space so we have an additional MINT_VTRESULT that copies the return around in the caller frame. In addition to this we had retval, is_void locals, saved as part of the current frame state which just make things confusing and are not needed.
This commit simplifies the call procedure by completely moving the responsability of the return to the called method. Simply put, once a method is ready for the call, it will leave its stack and vtstack to the state after pop-ing the arguments. The called method will then directly push the result to the parent frame's stack/vtstack. This means the calling code doesn't care about the type of the return anymore, the IL instructions after the call will just expect the return on the stack, if any, and use it naturally.
We still make use of frame->retval for interp entry frames (since we don't have a parent frame to push to), for pinvoke and jit_call frames out of convenience, but we should consider removing this variable to further trim down InterpFrame.