Generate calls to interface methods through resolve helper #112406
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This reverts commit 7f879d6.
|
For this not to be a throughput regression, we need to figure out a clean way to implement the hack in last two commits. I ran JSON serialization with/without this PR. Having to preserve argument registers before calling the resolve helper costs us about 1% in throughput. PR-hack is the hack where we allow RyuJIT to assume argument registers are not clobbered. We better not take a GC in the slow path.
Do we have a way to do this hack cleanly that doesn't involve building a new universal transition? |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
342fb19 to
b370b17
Compare
There was a problem hiding this comment.
Copilot reviewed 3 out of 22 changed files in this pull request and generated no comments.
Files not reviewed (19)
- src/coreclr/inc/corinfo.h: Language not supported
- src/coreclr/inc/jiteeversionguid.h: Language not supported
- src/coreclr/inc/jithelpers.h: Language not supported
- src/coreclr/jit/codegencommon.cpp: Language not supported
- src/coreclr/jit/gentree.cpp: Language not supported
- src/coreclr/jit/gentree.h: Language not supported
- src/coreclr/jit/lower.cpp: Language not supported
- src/coreclr/jit/morph.cpp: Language not supported
- src/coreclr/jit/targetamd64.h: Language not supported
- src/coreclr/jit/targetarm64.h: Language not supported
- src/coreclr/nativeaot/Runtime/AsmOffsets.h: Language not supported
- src/coreclr/nativeaot/Runtime/CMakeLists.txt: Language not supported
- src/coreclr/nativeaot/Runtime/EHHelpers.cpp: Language not supported
- src/coreclr/nativeaot/Runtime/StackFrameIterator.cpp: Language not supported
- src/coreclr/nativeaot/Runtime/amd64/DispatchResolve.asm: Language not supported
- src/coreclr/nativeaot/Runtime/amd64/UniversalTransition.asm: Language not supported
- src/coreclr/nativeaot/Runtime/arm64/DispatchResolve.asm: Language not supported
- src/coreclr/nativeaot/Runtime/arm64/UniversalTransition.asm: Language not supported
- src/coreclr/nativeaot/Runtime/inc/rhbinder.h: Language not supported
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs:267
- Ensure that the new helper CORINFO_HELP_INTERFACELOOKUP_FOR_SLOT is covered by tests to verify its behavior.
CORINFO_HELP_INTERFACELOOKUP_FOR_SLOT, // Resolve a non-generic interface method from this pointer and dispatch cell
|
@dotnet/ilc-contrib this is ready for review @jakobbotsch who would be the best to review the JIT side? |
Yes, I have pushed a commit to do that (some of the changes in that commit are quick hack that needs cleaning up if it works). |
Track all integer registers for calls in `regPtrDsc`. This does not cost any extra memory and it saves us from going back and forth between an intermediate format. It also unblocks proper GC reporting for helper calls that are GC reported with non-standard calling convention.
|
/azp run jit-cfg |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/samsung Could you please take a look? These changes may be related to riscv64. |
1 similar comment
|
@dotnet/samsung Could you please take a look? These changes may be related to riscv64. |
|
Should we just take a simpler but more obviously correct fix here instead of trying to make the calling convention change work? It does not appear to be that simple. I still wonder if it wouldn't be simpler/less risk to change the cached interface dispatch stubs to emit the call through the CFG dispatcher directly and avoid the more substantial changes in how the scheme works. |
Would that help? Right now the interface call sequence is: The call is indirected within the codegen so doing this in the stub that DispatchCellInMutableMemory points to would be too late because we already made a unverified call. |
Really? That would surprise me. JIT should make sure this goes through CFG. That should definitely be fixed. |
Ah, yeah, this goes through a CFG check. But your suggestion is to go through two CFG checks for a single interface call? |
Yes, it would end up going through two CFG checks. I don't know how bad the impact on performance would be. |
I can have a look at that next week. However, it looks like the resolve helper method would actually be a perf improvement from where we're now, so it's a bit more attractive (#112406 (comment)). |
I agree that resolve helper with different calling convention is likely the best way to do the dispatch when CFG is enabled, but at this point in the release cycle and based on the kind of issues we saw trying to make that work it seems like a lot of risk to me. Maybe the resolve helper with standard calling convention will turn out to be better than double CFG approach. That should be less risky (but not quite as low risk as changing the CID stubs). |
This change looks risky to me for .NET 10 at this point regardless of the implementation strategy. |
|
Could you please resolve the conflicts and get this build again so that we can start iterating on it? (It can wait after .NET 10 snap.) |
|
@dotnet/samsung Could you please take a look? These changes may be related to riscv64. |
Successor of dotnet#112406. That branch has way too many merge conflicts, so leaving the PR as-is for posterity and starting a new one with a clean slate (I basically manually ported everything). This changes interface calls when CFG is enabled to use a resolve helper instead of the stub dispatcher. I've also extended the CFG testing a bit to cover more of interface calls in the smoke test. ```csharp static int Call(IFoo f, int x, int y) => f.Call(x, y); ``` Before this change: ``` 00007FF605971D50 push rbx 00007FF605971D51 sub rsp,20h 00007FF605971D55 mov qword ptr [rsp+30h],rcx 00007FF605971D5A mov r10d,edx 00007FF605971D5D mov ebx,r8d 00007FF605971D60 mov rcx,qword ptr [__InterfaceDispatchCell_repro_Program_IFoo__Call_repro_Program__Call (07FF6059BEBD0h)] 00007FF605971D67 call qword ptr [__guard_check_icall_fptr (07FF6059CD558h)] 00007FF605971D6D mov rax,rcx 00007FF605971D70 mov rcx,qword ptr [rsp+30h] 00007FF605971D75 mov r8d,ebx 00007FF605971D78 mov edx,r10d 00007FF605971D7B lea r10,[__InterfaceDispatchCell_repro_Program_IFoo__Call_repro_Program__Call (07FF6059BEBD0h)] 00007FF605971D82 call rax 00007FF605971D84 nop 00007FF605971D85 add rsp,20h 00007FF605971D89 pop rbx 00007FF605971D8A ret ``` After this change: ``` 00007FF704181CF0 sub rsp,28h 00007FF704181CF4 lea r10,[__InterfaceDispatchCell_repro_Program_IFoo__Call_repro_Program__Call (07FF7041CEBD0h)] 00007FF704181CFB call RhpResolveInterfaceMethodFast (07FF703FFF5A0h) 00007FF704181D00 call qword ptr [__guard_dispatch_icall_fptr (07FF7041DD560h)] 00007FF704181D06 nop 00007FF704181D07 add rsp,28h 00007FF704181D0B ret ```
I opened #122639 because this had way too many merge conflicts to resolve and I didn't want to force push and lose history. |
Successor of #112406. That branch has way too many merge conflicts, so leaving the PR as-is for posterity and starting a new one with a clean slate (I basically manually ported everything). This changes interface calls when CFG is enabled to use a resolve helper instead of the stub dispatcher. I've also extended the CFG testing a bit to cover more of interface calls in the smoke test. ```csharp static int Call(IFoo f, int x, int y) => f.Call(x, y); ``` Before this change: ``` 00007FF605971D50 push rbx 00007FF605971D51 sub rsp,20h 00007FF605971D55 mov qword ptr [rsp+30h],rcx 00007FF605971D5A mov r10d,edx 00007FF605971D5D mov ebx,r8d 00007FF605971D60 mov rcx,qword ptr [__InterfaceDispatchCell_repro_Program_IFoo__Call_repro_Program__Call (07FF6059BEBD0h)] 00007FF605971D67 call qword ptr [__guard_check_icall_fptr (07FF6059CD558h)] 00007FF605971D6D mov rax,rcx 00007FF605971D70 mov rcx,qword ptr [rsp+30h] 00007FF605971D75 mov r8d,ebx 00007FF605971D78 mov edx,r10d 00007FF605971D7B lea r10,[__InterfaceDispatchCell_repro_Program_IFoo__Call_repro_Program__Call (07FF6059BEBD0h)] 00007FF605971D82 call rax 00007FF605971D84 nop 00007FF605971D85 add rsp,20h 00007FF605971D89 pop rbx 00007FF605971D8A ret ``` After this change: ``` 00007FF704181CF0 sub rsp,28h 00007FF704181CF4 lea r10,[__InterfaceDispatchCell_repro_Program_IFoo__Call_repro_Program__Call (07FF7041CEBD0h)] 00007FF704181CFB call RhpResolveInterfaceMethodFast (07FF703FFF5A0h) 00007FF704181D00 call qword ptr [__guard_dispatch_icall_fptr (07FF7041DD560h)] 00007FF704181D06 nop 00007FF704181D07 add rsp,28h 00007FF704181D0B ret ```
This changes interface calls when CFG is enabled to use a resolve helper instead of the stub dispatcher. I've also extended the CFG testing a bit to cover more of interface calls in the smoke test.
Before this change:
After this change: