-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Generate calls to interface methods through resolve helper #112406
base: main
Are you sure you want to change the base?
Changes from all commits
cb9785d
6bdcdb1
7f879d6
6d45447
9d0a3bd
2f0a285
6cd6567
36fa432
312d75d
5249b9a
b370b17
88eeeee
1d710c4
c3d49bb
66f6270
e6b1819
1ea2b34
bb89b87
5ca6411
5d66c77
f28053d
d866b1e
a70c650
bb2d70f
42343da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Unchanged files with check annotations Beta
<!-- Use IgnoreStandardErrorWarningFormat because Arcade sets WarnAsError and there's an existing warning in the native build. --> | ||
<Message Text="Executing "$(MSBuildThisFileDirectory)$(_CoreClrBuildScript)" @(_CoreClrBuildArg->'%(Identity)',' ')" Importance="High" /> | ||
<Exec Command=""$(MSBuildThisFileDirectory)$(_CoreClrBuildScript)" @(_CoreClrBuildArg->'%(Identity)',' ')" | ||
Check failure on line 105 in src/coreclr/runtime.proj
|
||
IgnoreStandardErrorWarningFormat="true" /> | ||
</Target> | ||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.