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.

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

Cleanup some code under FEATURE_STUBS_AS_IL #104731

Merged
merged 33 commits into from
Sep 19, 2024
Merged

Conversation

huoyaoyuan
Copy link
Member

@huoyaoyuan huoyaoyuan commented Jul 11, 2024

There's no actually no IL stubs under this feature switch now, only asm stubs.

This PR addresses the simple cases. On all platforms except Windows x86, single cast delegate is written in invariant assembly code.

Remaining code under the feature switch is COM call stub, which is more complicated and is better separated out.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 11, 2024
@jkotas jkotas changed the title Cleanup some code under FEATURE_STUBS_AS_AL Cleanup some code under FEATURE_STUBS_AS_IL Jul 12, 2024
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stubmgr.cpp Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Jul 17, 2024

Code quality

If this helps with code quality, could you please make the same change in

codeStream.EmitLdArg(0);
codeStream.Emit(ILOpcode.ldfld, emit.NewToken(functionPointerField.InstantiateAsOpen()));
as well? It is naot equivalent of this stub.

@huoyaoyuan
Copy link
Member Author

The same tests in DI are still failing. Can it be related to the signature instantiation in ILC?

if (method.OwningType.HasInstantiation)
{
// If the owning type is generic, the signature will contain T's and U's.
// We need !0's and !1's.
TypeDesc[] typesToReplace = new TypeDesc[method.OwningType.Instantiation.Length];
TypeDesc[] replacementTypes = new TypeDesc[typesToReplace.Length];
for (int i = 0; i < typesToReplace.Length; i++)
{
typesToReplace[i] = method.OwningType.Instantiation[i];
replacementTypes[i] = context.GetSignatureVariable(i, method: false);
}
TypeDesc[] parameters = new TypeDesc[method.Signature.Length];
for (int i = 0; i < parameters.Length; i++)
{
parameters[i] = method.Signature[i].ReplaceTypesInConstructionOfType(typesToReplace, replacementTypes);
}
TypeDesc returnType = method.Signature.ReturnType.ReplaceTypesInConstructionOfType(typesToReplace, replacementTypes);
signature = new MethodSignature(signature.Flags, signature.GenericParameterCount, returnType, parameters);
}
Is it also required for coreclr?

@jkotas
Copy link
Member

jkotas commented Jul 18, 2024

The same tests in DI are still failing. Can it be related to the signature instantiation in ILC?

We should not need a special signature instantiation like that in CoreCLR. I suspect that it is related to a special delegate calling convention used for delegate Invocation in some cases. The delegate target cannot be called via pointer - it must be called as Delegate.Invoke method so that the special calling convention kicks in.

Do you know if the Invoke method is used in the DI on a performance critical path?

This is proving to be much harder than what I thought it is going to be.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/coreclr/vm/dllimport.h Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.h Outdated Show resolved Hide resolved
src/coreclr/vm/stublink.h Outdated Show resolved Hide resolved
src/coreclr/vm/stublink.h Outdated Show resolved Hide resolved
src/coreclr/vm/method.hpp Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Sep 19, 2024

I have pushed a commit with a small change to handle the new IL stub in stub manager. The manual debugger test from #104731 (comment) works fine now.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@jkotas
Copy link
Member

jkotas commented Sep 19, 2024

Remaining code under the feature switch is COM call stub, which is more complicated and is better separated out.

Do you plan to work on cleaning up the remaining code under the feature switch?

@huoyaoyuan huoyaoyuan deleted the stubs-as-il branch September 19, 2024 16:18
@huoyaoyuan
Copy link
Member Author

Do you plan to work on cleaning up the remaining code under the feature switch?

Aaron suggested me to avoid converting COM interop related code because the effort needed for reviewing. I'm already busy now until October.

If you can confirm the review effort on behalf of interop team, I can start to investigate this when I finish urgent tasks.

@jkotas
Copy link
Member

jkotas commented Sep 20, 2024

I gave it a shot in #108048.

sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
* Use direct SinglecastDelegateInvokeStub on windows x86

* Cleanup ifdefs

* EmitDebugBreak is unused

* Use IL stub for Delegate.Invoke

* Remove SinglecastDelegateInvokeStub in asm

* Fix method name and argument count

* Code quality

* Delete JIT_InternalThrow and CorInfoException

* Apply the same codegen to ilc

* Add KeepAlive

* Emit recursive call instead

* Add tests for indirect delegate invocation

* Use public reflection instead.

* Use ldvirtftn to bypass this check

* Use winner from CompareExchange

* Return Stub from GetInvokeMethodStub

* Delete NEWSTUB_FL_MULTICAST and dependents

* Handle new stub in ILStubManager

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

4 participants