Skip to content

[CI run only] Enable FEATURE_INSTANTIATINGSTUB_AS_IL on win-x86 #116331

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

Closed

Conversation

filipnavara
Copy link
Member

Seems like it works and passes tests, let's verify that. Needs some benchmarking.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 5, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 5, 2025
@filipnavara
Copy link
Member Author

@huoyaoyuan Any thoughts on how to benchmark this? Asking you since you worked on enabling the other FEATURE_*_AS_IL features earlier.

@filipnavara filipnavara marked this pull request as draft June 5, 2025 07:28
@am11 am11 added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 5, 2025
@filipnavara
Copy link
Member Author

@EgorBot -windows_intel -use32bit

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

public class SimpleBench
{
   [Benchmark]
   public void InvokeGenericDelegate() => (new Action<Object>(ActionFunction)).Invoke(1);

   void ActionFunction<T>(T obj) { }
}

@huoyaoyuan
Copy link
Member

@huoyaoyuan Any thoughts on how to benchmark this? Asking you since you worked on enabling the other FEATURE_*_AS_IL features earlier.

First I just viewed the generated assembly with DOTNET_Jitdisasm before doing benchmark.
For these type of feature that involves signature handling, I'd test something complex, like #104192 (comment)

@filipnavara
Copy link
Member Author

Thanks, that helped. Seems like I will need to investigate if we can make the tailcalls work (ie. no FEATURE_FASTTAILCALL at the moment).

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
@filipnavara
Copy link
Member Author

filipnavara commented Jun 11, 2025

Closing this for now, there's too many paper cuts. I'll save the good bits of x86 fast tailcall in a branch for some other time. It's not too far off from working but even if it did work there are code quality issues that need to be resolved first. Analyzing the JIT output revealed three separate problems:

  1. Unnecessary EBP frames - addressing that in Exclude tail call counts from check to prevent unnecessary EBP frame #116413
  2. When tailcalling a method with struct on stack we copy the argument through PartialRepInstr. This copy and related register allocation happens even if the source and target location of the argument is the same. That's particularly more pronounced on x86 where Span<T> happens to fall into this case. (cc @dotnet/jit-contrib, this would be nice to fix for all platforms)
  3. Even the simplest calli happens to be represented by mov eax, <address>; jmp eax. This is not necessarily the end of the world. We generate the same pattern in IL stubs on x64 now. On x86 we do generate jmp near <address> instead in the IL stubs. Seems like this micro-optimization affects the benchmarks on certain Intel processors even if I would expect the effect to be negligable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants