[clr-interp] Add support for virtual method calls #114529
Conversation
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
| { | ||
| pInterpreterFrame->SetTopInterpMethodContextFrame(pFrame); | ||
| GCX_PREEMP(); | ||
| pMD->PrepareInitialCode(CallerGCMode::Coop); |
There was a problem hiding this comment.
Why is it CallerGCMode::Coop if we just did GCX_PREEMP?
There was a problem hiding this comment.
This is setting the caller GC mode for the code being prepared, not the caller mode of the PrepareInitialCode.
| if (isVirtual) | ||
| { | ||
| AddIns(INTOP_CALLVIRT); | ||
| m_pLastNewIns->data[0] = GetDataItemIndex(targetMethod); |
There was a problem hiding this comment.
Why are we using MethodDataItemIndex in one arm and DataItemIndex in the other if it's a CORINFO_METHOD_HANDLE in both cases?
There was a problem hiding this comment.
The INTOP_CALL MethodDesc slot contains a tagged pointer. Initially it is a tagged MethodDesc, afterwards it is meant to be replaced with the code pointer. In the case of INTOP_CALLVIRT it is just a MethodDesc without any tag.
I think this should change in the future. Some of the changes might look like this: In the case of INTOP_CALL we might want to add the address of the MethodDesc precode slot (that will get patched with either a compiled or interpreter code pointer). For vtable virtual calls we might have the offsets to the vtable slot containing the code pointer. For interface calls we might maintain a cache in the instruction stream similar to VSD (helping with monomorphic calls) and also store the interface method token for resolving the method to be called as a fallback.
I think you want to mirror what we do for virtual method dispatch in regular JITed code. Some combination of:
I do not think it is worth it to try to invent some super optimized path for regular virtual methods that uses slot indices. I think it would just create a lot of problems with keeping things unified between runtime that has interpreter support and runtime that does not have interpreter support. The caches above should be just fine. We have actually tried to use caches like above for regular virtual methods as well at one point in the past and the performance was about as good on average as the performance of invocation using slot indices. |
|
I think normal virtual calls (that are dispatch through the vtable) should still go through the vtable in the interpreter scenario long term. A vtable slot would contain either a normal code pointer (for compiled code) or a tagged interpreter IR code pointer (for interpreted code). So this type of call would look logically identical between interp and jit. They would both load the code pointer from the slot and invoke it accordingly whether the code pointer is jit or interp. I don't see why we would want to use a different approach on interpreter (via hash tables or per call site caching). Although for interface calls I see us doing something like this, to reuse as much as possible the logic/code from VSD. |
|
Well, I guess the obvious drawback would be that each virtual call emitted with JIT would need to check if the address to call is tagged. |
It will just work once you implement the general path and I expect that it will work reasonably well. We use the per call site caching for regular virtual methods in regular CoreCLR for less common cases too.
Right. It is a performance trade-off. You will shift the costs around. It is not clear to me whether it would be a win at the end in the typical configuration. |
BrzVlad
force-pushed
the
feature-clr-interp7
branch
from
feefc96 to
722b18c
Compare
This follows a trivial approach where we resolve the MethodDesc of the target method by reusing existing functionality. Long term, virtual dispatching should go through slots continaing interpreter IR code pointer, in a similar fashion to JIT.
No gc support yet
BrzVlad
force-pushed
the
feature-clr-interp7
branch
from
3c3d613 to
f367573
Compare
The PR follows the suggestion of @davidwrighton (thanks a lot for the input), which makes it trivial to implement these types of calls, by reusing existing code to resolve the target MethodDesc that needs to be invoked. This should help us expand interpreter capability quite fast, given I expect it to work for pretty much all scenarios of virtual/interface calls.
Interpreter execution design is optimized for invocation based solely on interpreter IR code pointer. This means that, long term, we should switch to invocation based on method slots that are expected to contain directly the code pointer, without having to do all these lookups at MethodDesc level. This seems that it might be a bit tricky to do and I'm investigating what options we would have.