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

Parse CallConvSwift in CoreCLR/NativeAOT #96707

Merged
merged 16 commits into from
Jan 30, 2024

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 9, 2024

Add support for recognizing CallConvSwift in CoreCLR and the managed type system to pass to the JIT. Mark the Swift* types as intrinsics.

Also, improve some rough edges around CallConv parsing in the managed type system around UnmanagedCallersOnly APIs

@jkoritzinsky jkoritzinsky added this to the 9.0.0 milestone Jan 9, 2024
@ghost ghost assigned jkoritzinsky Jan 9, 2024
…he calling convention isn't in the method signature)
@jkoritzinsky jkoritzinsky marked this pull request as ready for review January 26, 2024 20:14
@@ -130,6 +130,18 @@ public static bool IsMarshallingRequired(MethodDesc targetMethod)
return false;
}

public static bool IsMarshallingRequired(MethodSignature methodSig, ModuleDesc moduleContext, UnmanagedCallingConventions callingConvention)
Copy link
Member

Choose a reason for hiding this comment

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

The callingConvention parameter is not used, is it intentional?

Why do we need a new overload though? I would expect the existing IsMarshallingRequired to get an extra callingConvention argument if answer to this now depends on the calling convention as well (or why can we still decide it without the calling convention in the other overload?).

Copy link
Member Author

Choose a reason for hiding this comment

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

In the other overload, we can extract the calling convention from the MethodSignature. This overload is for UnmanagedCallersOnly methods where the callconv is in the attribute, not the managed signature.

Copy link
Member

Choose a reason for hiding this comment

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

The parameter is still unused, is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's intentional that it's unused.

@ryujit-bot
Copy link

Diff results for #96707

Throughput diffs

Throughput diffs for windows/arm64 ran on linux/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch +0.01%

Details here


@ryujit-bot
Copy link

Diff results for #96707

Throughput diffs

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch +0.01%

Details here


Throughput diffs for linux/x64 ran on linux/x64

MinOpts (-0.01% to 0.00%)
Collection PDIFF
realworld.run.linux.x64.checked.mch -0.01%

Details here


@ryujit-bot
Copy link

Diff results for #96707

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch -0.01%

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch -0.01%

Details here


Throughput diffs for linux/x64 ran on linux/x64

MinOpts (-0.01% to -0.00%)
Collection PDIFF
realworld.run.linux.x64.checked.mch -0.01%
smoke_tests.nativeaot.linux.x64.checked.mch -0.01%

Details here


@ryujit-bot
Copy link

Diff results for #96707

Throughput diffs

Throughput diffs for linux/x64 ran on linux/x64

MinOpts (-0.01% to -0.00%)
Collection PDIFF
realworld.run.linux.x64.checked.mch -0.01%
smoke_tests.nativeaot.linux.x64.checked.mch -0.01%

Details here


@jkoritzinsky jkoritzinsky merged commit f48a99e into dotnet:main Jan 30, 2024
191 of 197 checks passed
@jkoritzinsky jkoritzinsky deleted the callconv-swift-coreclr branch January 30, 2024 22:52
@@ -135,6 +135,25 @@ public static UnmanagedCallingConventions GetPInvokeMethodCallingConventions(thi
return result;
}

public static UnmanagedCallingConventions GetDelegateCallingConventions(this TypeDesc delegateType)
Copy link
Member

Choose a reason for hiding this comment

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

This method doesn't appear used. Is that intentional?

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 removed the usage of it during a refactor. I think it's still useful to have, but we can remove it and re-add later when we have a use case.

{
Debug.Assert(delegateType.IsDelegate);

if (delegateType is EcmaType ecmaDelegate)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Might want to either do delegateType.GetTypeDefinition() is EcmaType ecmaDelegate or assert the type is not generic so that we just don't do bad codegen if we ever allow interop with generic delegates.

@@ -130,6 +130,18 @@ public static bool IsMarshallingRequired(MethodDesc targetMethod)
return false;
}

public static bool IsMarshallingRequired(MethodSignature methodSig, ModuleDesc moduleContext, UnmanagedCallingConventions callingConvention)
Copy link
Member

Choose a reason for hiding this comment

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

The parameter is still unused, is that intentional?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants