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

Don't forget to pass instantiation context after devirtualizing #51918

Closed
wants to merge 1 commit into from

Conversation

MichalStrehovsky
Copy link
Member

This is a difference between CoreCLR and the managed type system - the result of interface method resolution is going to be a canonical method on the CoreCLR side, but exact on the managed side.

So this implementation is slightly different from how it looks like on the VM side. We slightly touched on the difference in #45744 but there was more to it.

This is a difference between CoreCLR and the managed type system - the result of interface method resolution is going to be a canonical method on the CoreCLR side, but exact on the managed side.

So this implementation is slightly different from how it looks like on the VM side. We slightly touched on the difference in dotnet#45744 but there was more to it.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 27, 2021
@MichalStrehovsky
Copy link
Member Author

@dotnet/crossgen-contrib PTAL

@agocke This is the issue you hit with NativeAOT in Roslyn. I assume Roslyn proper doesn't hit this because it's either not compiled with crossgen2, or because version bubble crossing happens (aborting the devirt).

<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<CLRTestPriority>0</CLRTestPriority>
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 want to make this Pri 1, but I want this to run in the CI first.

@jkotas jkotas added area-crossgen2-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Apr 27, 2021
@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Apr 27, 2021

Huh, so RyuJIT then goes and feeds the result of the devirt back into resolveVirtualMethod for... reasons?

We resolve System.Collections.Generic.ICollection1<int32>.get_Count on System.Collections.Generic.Dictionary2+KeyCollection<int32,string> into System.Collections.Generic.Dictionary2+KeyCollection<int32,System.__Canon>.get_Count().

And then RyuJIT asks us to resolveVirtualMethod the System.Collections.Generic.Dictionary2+KeyCollection<int32,System.__Canon>.get_Count() on System.Collections.Generic.Dictionary2+KeyCollection<int32,string>?

This now fails because we're not the CoreCLR type system where <int32,System.__Canon> is pretty much <int32,string> in arbitrary ways.

We'll need to also canonicalize in some other spot too. I haven't figured out where yet.

@MichalStrehovsky
Copy link
Member Author

Sigh, so this is now failing because the devirtualization logic has other problems with canonical code unrelated to what I'm fixing here:

using System;
using System.Runtime.CompilerServices;

Console.WriteLine(Foo<Atom1>.Call());
Console.WriteLine(Foo<Atom2>.Call());


interface IFooable<T>
{
    public string DoFoo(T x);
}

class Base : IFooable<Atom2>
{
    string IFooable<Atom2>.DoFoo(Atom2 x) => "Base";
}

sealed class Foo<T> : Base, IFooable<T>
{
    string IFooable<T>.DoFoo(T x) => "Foo";

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static string Call()
    {
        var f = new Foo<T>();
        return ((IFooable<T>)f).DoFoo(default) + ((IFooable<Atom2>)f).DoFoo(null);
    }
}

class Atom1 { }
class Atom2 { }

This is supposed to print:

FooBase
FooFoo

However, after crossgen2, it will print

FooBase
FooBase

This will need a bit more rewriting.

I'm a bit concerned about the amount of test coverage we have for type system corner cases like this.

@MichalStrehovsky
Copy link
Member Author

Oh so the second repro program is also broken on CoreCLR without ReadyToRun at all. I was hoping I can just steal whatever CoreCLR is doing but nope.

I'll watch this from the sidelines. Filed #51983 and #51982.

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
davidwrighton added a commit that referenced this pull request Jun 11, 2021
Address deficiencies in current devirtualization infrastructure

- Remove the responsibility of creating a CORINFO_RESOLVED_TOKEN structure from the JIT and make it a responsibility of the VM side of the jit interface.
  - This enables the component (crossgen2) which has deeper understanding of the requirements here to correctly handle scenarios that would otherwise require expressing crossgen2 specific details across the jit interface.
- Add a new set of fixups (`READYTORUN_FIXUP_Check_VirtualFunctionOverride` and `READYTORUN_FIXUP_Verify_VirtualFunctionOverride`) these are used to validate that the behavior of the runtime and crossgen2 compiler is equivalent for a virtual resolution event
  - `READYTORUN_FIXUP_Check_VirtualFunctionOverride` will ensure that the virtual resolution decision is the same at crossgen2 time and runtime, and if the decision differs, any generated code affected by the decision will not be used.
  - `READYTORUN_FIXUP_Verify_VirtualFunctionOverride` will perform the same checks as `READYTORUN_FIXUP_Check_VirtualFunctionOverride`, but if it fails the check, the process will be terminated with a fail-fast. It is intended for use under the `--verify-type-and-field-layout` stress mode.
  - Currently only the `READYTORUN_FIXUP_Verify_VirtualFunctionOverride` is actually generated, and it is only generated when using the `--verify-type-and-field-layout` switch to crossgen2. Future work will identify if there are scenarios where we need to generate the `READYTORUN_FIXUP_Check_VirtualFunctionOverride` flag. One area of possible concern is around covariant returns, another is around handling of type equivalence.
- In order to express the fixup signature for the VirtualFunctionOverride fixups, a new flag has been added to `ReadyToRunMethodSigFlags`. `READYTORUN_METHOD_SIG_UpdateContext` will allow the method signature to internally specify the assembly which is associated with the method token, instead of relying on the ambient context.
- R2RDump and the ReadyToRun format documentation have been updated with the details of the new fixups/flags.
- Update the rules for handling unboxing stubs
  - See #51918 for details. This adds a new test, as well as proper handling for unboxing stubs to match the JIT behavior
  - Also revert #52605, which avoided the problem by simply disabling devirtualization in the presence of structs
- Adjust the rules for when it is legal to devirtualize and maintain version resiliency
  - The VersionsWithCode and VersionsWithType rules are unnecessarily restrictive.
  - Instead Validate that the metadata is safely checkable, and rely on the canInline logic to ensure that no IL that can't be handled is inlined.
  - This also involved adding a check that the chain of types from the implementation type to the declaration method table type is within the version bubble.
  - And changing the `VersionsWithType` check on the implementation type, to a new `VersionsWithTypeReference` check which can be used to validate that the type can be referred to, in combination with using `VersionsWithType` on the type definition.
  - By adjusting the way that the declMethod is referred to, it becomes possible to use the declMethod without checking the full method is `VersionsWithCode`, and it just needs a relationship to version matching code.
- In `resolveVirtualMethod` generate the `CORINFO_RESOLVED_TOKEN` structures for the jit
  - In particular we are now able to resolve to methods where the decl method is the resolution result but is not within the version bubble itself. This can happen if we can prove that the decl method is the only method which can possibly implement a virtual.
- Add support for devirtualization reasons to crossgen2
- Port all devirtualization abort conditions to crossgen2 from runtime that were not already present
- Fix devirtualization from a canonical virtual method when the actual implementation is more exact
- Fix variant interface override scenario where there is an interface that requires implementation of the variant interface as well as the variant interface itself.
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
@MichalStrehovsky MichalStrehovsky deleted the devirtcontext branch July 17, 2022 23:53
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.

4 participants