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

Rework RhGetRuntimeHelperForType in C# #1371

Conversation

kant2002
Copy link
Contributor

@kant2002 kant2002 commented Aug 2, 2021

as per this discussion #1294 (comment)

This is required to make compile on ARM
@jkotas
Copy link
Member

jkotas commented Aug 2, 2021

You may also need to change GenericLookupResultReferenceType LookupResultReferenceType(NodeFactory factory) for affected dictionary cell types to return GenericLookupResultReferenceType.Direct .

Technically there no indirecion marcro left, but interesting, should I keep GenericLookupResultReferenceType as it is. Probably this is some core feature which used occasionallly to get tactical results
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

There is one more area that needs adjustment - the runtime type loader.

Search for HelperForType in src\coreclr\nativeaot\System.Private.TypeLoader. The places that try to dereference the result should no longer do that. There should be 4 hits.

[MethodImplAttribute(MethodImplOptions.InternalCall)]
[RuntimeImport(RuntimeLibrary, "RhGetRuntimeHelperForType")]
internal static extern unsafe IntPtr RhGetRuntimeHelperForType(EETypePtr pEEType, RuntimeHelperKind kind);
internal static unsafe IntPtr RhGetRuntimeHelperForType(EETypePtr pEEType, RuntimeHelperKind kind)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of inlining this into RuntimeImports.cs, could you put the implementation as a RuntimeExport into src\coreclr\nativeaot\Runtime.Base\src\System\Runtime\RuntimeExports.cs? Use EEType* instead of EETypePtr there.

RuntimeHelperKind enum will have to move to src\coreclr\tools\Common\Internal\Runtime\RuntimeConstants.cs but that's a better spot for it.

Then you can keep this InternalCall as-is.

@kant2002
Copy link
Contributor Author

kant2002 commented Aug 3, 2021

Tests do not run for some reason.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Looks good otherwise! Thanks!

@@ -294,7 +294,7 @@ internal override IntPtr Create(TypeBuilder builder)
internal override unsafe IntPtr CreateLazyLookupCell(TypeBuilder builder, out IntPtr auxResult)
{
auxResult = IntPtr.Zero;
return *(IntPtr*)Create(builder);
Copy link
Member

Choose a reason for hiding this comment

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

This one is still indirected.

@@ -57,4 +57,14 @@ internal static class SpecialDispatchMapSlot
public const ushort Diamond = 0xFFFE;
public const ushort Reabstraction = 0xFFFF;
}

// Keep in sync with RH\src\rtu\runtimeinstance.cpp
Copy link
Member

Choose a reason for hiding this comment

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

We only have one copy of this now - no longer need to keep in sync with anything.

return (IntPtr)(delegate*<EEType*, object, void>)&TypeCast.CheckVectorElemAddr;

default:
throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

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

Throwing from here won't lead to anything good. Maybe assert and return IntPtr.Zero?

@@ -450,5 +450,65 @@ public static int RhGetThunkSize()
{
return InternalCalls.RhpGetThunkSize();
}

[MethodImplAttribute(MethodImplOptions.InternalCall)]
Copy link
Member

Choose a reason for hiding this comment

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

The MethodImpl is not needed.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit 22a27c8 into dotnet:feature/NativeAOT Aug 4, 2021
@kant2002 kant2002 deleted the kant/rework-getruntimehelper branch August 4, 2021 02:36
MichalStrehovsky added a commit to MichalStrehovsky/runtimelab that referenced this pull request Sep 8, 2021
jkotas pushed a commit that referenced this pull request Sep 8, 2021
* Get rid of indirection in fat function pointers

* Clean up after #1371

* Get rid of indirections around statics

* Delete IndirectionNode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants