Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generate calls to interface methods through resolve helper #112406

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Generate calls to interface methods through resolve helper #112406

wants to merge 12 commits into from

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Feb 11, 2025 •
edited
Loading

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.

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

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

This was referenced Feb 11, 2025
Open
Open
Open
@MichalStrehovsky MichalStrehovsky mentioned this pull request Feb 11, 2025
Closed
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

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.
PR is this PR without that hack.
Baseline is the vanilla CFG build.

Style Attemp1 ms Attemp2 ms Attemp3 ms Attemp4 ms Attemp5 ms
PR-hack 1103 1105 1096 1102 1103
PR 1125 1126 1139 1127 1134
Baseline 1112 1105 1111 1104 1106

Do we have a way to do this hack cleanly that doesn't involve building a new universal transition?

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review February 18, 2025 07:17
@Copilot Copilot bot review requested due to automatic review settings February 18, 2025 07:17
Copilot
Copilot AI reviewed Feb 18, 2025
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@MichalStrehovsky
Copy link
Member Author

@dotnet/ilc-contrib this is ready for review

@jakobbotsch who would be the best to review the JIT side?

jakobbotsch
jakobbotsch reviewed Feb 18, 2025
View reviewed changes
src/coreclr/inc/jithelpers.h
@@ -309,6 +309,7 @@
JITHELPER(CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT_TRACK_TRANSITIONS, JIT_ReversePInvokeExitTrackTransitions, METHOD__NIL)

JITHELPER(CORINFO_HELP_GVMLOOKUP_FOR_SLOT, NULL, METHOD__NIL)
JITHELPER(CORINFO_HELP_INTERFACELOOKUP_FOR_SLOT, NULL, METHOD__NIL)
Copy link
Member

@jakobbotsch jakobbotsch Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to add an implementation for coreclr? Currently jit-cfg is likely to be broken by this change. It would be nice to make it work without having to make the JIT side codegen different from NAOT pattern.

Alternatively I guess it would be even more ideal if we switched the testing to a CFG-enabled outerloop NAOT run now that the support is more mature... But that's probably a bit more work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought CoreCLR doesn't even do interface dispatch in this shape? I thought #111771 tries to bring it over, but for some very limited scenarios. Are the interface codepaths even exercised under CoreCLR?

We already have a CFG run on native AOT but only on x64. It should be easy enough to add arm64 one. But we don't have any stress modes for either GC or JIT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not totally sure what the difference between CID and VSD is, but from the JIT codegen standpoint it looks identical to me.

In the worst case I would be ok with guarding the transformation in lowering under a NAOT check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas if I wanted to implement a CORINFO_HELP_INTERFACELOOKUP_FOR_SLOT on CoreCLR, would I need to somehow call VirtualCallStubManager::ResolveWorker? It's not exactly clear to me where I would find the parameters to call it so wondering if there's a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a commit into this PR that shows what it may look like on win x64. It calls the slow resolve helper every time. If it is too slow for the test, it is possible to improve the perf by adding a NAOT-like fast path once David's PR #111771 goes in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a commit into this PR that shows what it may look like on win x64

Thank you ❤️! I'll kick off a JIT-cfg run to see if the perf is acceptable. I think we'd basically only care if this more than quadruples the time it takes for the CI to run. These legs run very infrequently.

@MichalStrehovsky
Copy link
Member Author

/azp list

@azure-pipelines Azure Pipelines
Copy link

CI/CD Pipelines for this repository:

@MichalStrehovsky
Copy link
Member Author

/azp run jit-cfg

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Feb 18, 2025

Runtime changes LGTM

@MichalStrehovsky
Copy link
Member Author

/azp run jit-cfg

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkotas jkotas jkotas left review comments

@jakobbotsch jakobbotsch jakobbotsch left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@MichalStrehovsky MichalStrehovsky

Labels
area-NativeAOT-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MichalStrehovsky @jkotas @jakobbotsch