Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

[WIP] Unboxing for C++ backend#3670

Merged
jkotas merged 1 commit intodotnet:masterfrom
benpye:cpp-unboxing
May 24, 2017
Merged

[WIP] Unboxing for C++ backend#3670
jkotas merged 1 commit intodotnet:masterfrom
benpye:cpp-unboxing

Conversation

@benpye
Copy link
Contributor

@benpye benpye commented May 22, 2017 •
edited
Loading

This change set adds limited unboxing support for the C++ backend. Calling interface methods on boxed value types is functional, as is calling methods such as Convert.ToInt32 with a boxed value type. Enums (as in #2884) still fails, I need to look at what is going on with FindInterfaceMethodImplementationTarget.

@jkotas

Working examples:

using System;

internal class Program
{
    interface IMyInterface
    {
        void MyMethod();
    }
    struct MyValueType : IMyInterface
    {
        void IMyInterface.MyMethod()
        {
            Console.WriteLine("IMyInterface.MyMethod");
        }
    }
    private static void Main()
    {
        object o = new MyValueType();
        ((IMyInterface)o).MyMethod();
    }
}
using System;

internal class Program
{
    private static void Main()
    {
        Console.WriteLine(Convert.ToInt32((object)1));
    }
}

@dnfclas
Copy link

dnfclas commented May 22, 2017

@benpye,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

jkotas
jkotas reviewed May 22, 2017
View reviewed changes
src/ILCompiler.CppCodeGen/src/CppCodeGen/CppWriter.cs Outdated

for (int i = 0; i < argCount; i++)
{
if(i == 0 && hasThis && unbox)
Copy link
Member

@jkotas jkotas May 22, 2017

Choose a reason for hiding this comment

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

Would it be better to just check if (i == 0 && unbox) and then assert that hasThis is true?

src/ILCompiler.CppCodeGen/src/CppCodeGen/CppWriter.cs Outdated
/// </summary>
/// <param name="unboxingStubNode">The unboxing node to be output</param>
/// <param name="methodImplementations">The buffer in which to write out the C++ code</param>
private void OutputUnboxNode(UnboxingStubNode unboxingStubNode)
Copy link
Member

@jkotas jkotas May 22, 2017

Choose a reason for hiding this comment

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

Nit: We call these things unboxing stubs - OutputUnboxingStubNode or OutputUnboxingStub would be better name.

Copy link
Member

@MichalStrehovsky MichalStrehovsky May 22, 2017

Choose a reason for hiding this comment

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

Would it be possible to just allocate a normal CppMethodCodeNode for these, where _methodCode is this code and _dependencies points to the CppMethodCodeNode of the target?

It would fix the TODO here:

corert/src/ILCompiler.CppCodeGen/src/Compiler/DependencyAnalysis/CppCodegenNodeFactory.cs

Line 35 in e391acf

return new UnboxingStubNode(method);

It feels odd to allocate the UnboxingStubNode (which represents a machine code helper) and then try to patch things up later.

Copy link
Contributor Author

@benpye benpye May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The unboxing stubs define their own mangled name. We could derive CppMethodCodeNode, providing the unboxing names. Would have to swap GetCppMethodName for method.GetMangledName in several places I think, or at least redefine GetCppMethodName to use the getter on the method.

EDIT:
We also run into trouble in getting the right forward declarations this way, we rely on the MethodDesc being accurate. Is there a suitable MethodDesc type allowing the name to be redefined? I guess then we could avoid an unboxing specific node.

@benpye
Copy link
Contributor Author

benpye commented May 22, 2017 •
edited
Loading

Will address the two comments.

Is it possible to trace through the RyuJIT compiled code with any symbols. In the enum case it seems that FindImplSlotForCurrentType (at https://github.com/dotnet/corert/blob/master/src/Runtime.Base/src/System/Runtime/DispatchResolve.cs#L51 ) fails until pCur is NULL, and we return 0. Would be useful to see the expected behaviour to compare.

Seems RyuJIT takes a vastly different path to resolve. The following is still true:
In the enum case it seems that FindImplSlotForCurrentType (at https://github.com/dotnet/corert/blob/master/src/Runtime.Base/src/System/Runtime/DispatchResolve.cs#L51 ) fails until pCur is NULL, and we return 0.

jkotas
jkotas reviewed May 23, 2017
View reviewed changes
src/ILCompiler.CppCodeGen/src/CppCodeGen/CppWriter.cs Outdated
System.Diagnostics.Debug.Assert(hasThis);

var thisType = method.OwningType;
if (thisType.IsValueType)
Copy link
Member

@jkotas jkotas May 23, 2017

Choose a reason for hiding this comment

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

I think that this should be assert instead as well - unboxing stubs are only valid for value types.

@jkotas
Copy link
Member

jkotas commented May 23, 2017

@benpye Do you plan to do more changes for this? I think this change is improvement as is - I would be happy to merge it. The cleanup around the unboxing stub node define can be done separately.

@benpye
Copy link
Contributor Author

benpye commented May 24, 2017

@jkotas Happy to merge this as is, as you say, already an improvement. Have addressed your comments and squashed. Can work on cleaning up and what is going on with the enum case.

@jkotas jkotas merged commit fe602ed into dotnet:master May 24, 2017
@jkotas
Copy link
Member

jkotas commented May 24, 2017

Thanks!

@benpye benpye mentioned this pull request May 24, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jkotas jkotas jkotas left review comments

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-already-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benpye @dnfclas @jkotas @MichalStrehovsky