Account for CORINFO_HELP_VIRTUAL_FUNC_PTR in GT_LABEL; avoid double resolving #87395
Conversation
…esolving In the helper-based tailcall mechanism it is possible that we expand the target call into two actual calls: first, a call to CORINFO_HELP_VIRTUAL_FUNC_PTR to compute the target, and second a call to that target. We were not taking into account that the return address needed for the tailcall mechanism needs to be from the second call. In this particular case the runtime does not request the JIT to pass the target; that means we end up resolving the target from both the caller and from the CallTailCallTarget stub. Ideally the JIT would be able to eliminate the CORINFO_HELP_VIRTUAL_FUNC_PTR call in the caller since it turns out to be unused, but that requires changes in DCE (and is somewhat non-trivial, as we have to preserve a null-check). A simpler way to improve the case is to just change the runtime to always request the target from the JIT for GVMs, which means the CallTailCallTarget stub no longer needs to resolve it. That also has the effect of fixing the original issue, but I have left the original fix in as well. Fix dotnet#87393
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIn the helper-based tailcall mechanism it is possible that we expand the target call into two actual calls: first, a call to CORINFO_HELP_VIRTUAL_FUNC_PTR to compute the target, and second a call to that target. We were not taking into account that the return address needed for the tailcall mechanism needs to be from the second call. In this particular case the runtime does not request the JIT to pass the target; that means we end up resolving the target from both the caller and from the CallTailCallTarget stub. Ideally the JIT would be able to eliminate the CORINFO_HELP_VIRTUAL_FUNC_PTR call in the caller since it turns out to be unused, but that requires changes in DCE (and is somewhat non-trivial, as we have to preserve a null-check). A simpler way to improve the case is to just change the runtime to always request the target from the JIT for GVMs, which means the CallTailCallTarget stub no longer needs to resolve it. That also has the effect of fixing the original issue, but I have left the original fix in as well. Fix #87393
|
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
cc @dotnet/jit-contrib PTAL @kunalspathak. Also cc @jkotas for the small VM change |
|
VM change LGTM |
kunalspathak
left a comment
kunalspathak
left a comment
There was a problem hiding this comment.
I would probably rename genDefinePendingCallLabel() to genTryDefinePendingCallLabel(), the method will not always define it.
"Try" to me implies it can fail to define it, but it can't, it just only defines it when it is necessary. |
3 participants
In the helper-based tailcall mechanism it is possible that we expand the target call into two actual calls: first, a call to CORINFO_HELP_VIRTUAL_FUNC_PTR to compute the target, and second a call to that target. We were not taking into account that the return address needed for the tailcall mechanism needs to be from the second call.
In this particular case the runtime does not request the JIT to pass the target; that means we end up resolving the target from both the caller and from the CallTailCallTarget stub. Ideally the JIT would be able to eliminate the CORINFO_HELP_VIRTUAL_FUNC_PTR call in the caller since it turns out to be unused, but that requires changes in DCE (and is somewhat non-trivial, as we have to preserve a null-check).
A simpler way to improve the case is to just change the runtime to always request the target from the JIT for GVMs, which means the CallTailCallTarget stub no longer needs to resolve it. That also has the effect of fixing the original issue, but I have left the original fix in as well.
Fix #87393