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

Crossgen2 support for static virtual method resolution #54063

Closed
wants to merge 11 commits into from

Conversation

trylek
Copy link
Member

@trylek trylek commented Jun 11, 2021

This is my initial attempt at porting the CoreCLR runtime code
for SVM resolution to Crossgen2. The TypeHierarchyTest now fails
at 66-th scenario. The biggest problems I'm hitting are around
making sure that we drop the constraint from the actual entrypoint
import cells, otherwise we end up with constructs like

BaseScenario61.Method() @ DerivedScenario61

and that crashes the runtime. I guess that my treatment of generics
is most likely still incorrect.

Thanks

Tomas

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This logic that refers to Canon, and canonicalization is specific to actual compilation with utilizing a Canonical representation. I would either keep this in CorInfoImpl.ReadyToRun.cs or move move this to the DevirtualizationManager.

Copy link
Member

Choose a reason for hiding this comment

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

Actually given some more thought, this should definitely be part of DevirtualizationManager, and we probably need to have a different result for the conditions of failure due to various canonical shenanigans, and failure due to the resolution algorithm failing.

Note, the canonicalization failure stuff should be in the Common version of DevirtualizationManager.

We should also take a very close look at the version resilience characteristics of all of this, and consider if there are scenarios where we need to abort the calculation, and fallback to a RequiresRuntimeJitException. (That logic would go in the R2R specific DevirtualizationManager.

The resolution algorithm failing should trigger a CodeGenerationException, or RequiresRuntimeJitException (and also probably an assert), and the canonical failure, should trigger the change to use CORINFO_CALL_POINTER.

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 have moved the core logic to DevirtualizationManaged and I have added initial provisions for version bubble checks. I think they aren't perfect but they're better than nothing. I'll work on further refining them.

@davidwrighton
Copy link
Member

                        : DictionaryEntryKind.MethodEntrySlot);

As per PR #52173, this will need updating to use a ConstrainedMethodEntrySlot when a call or ldftn instruction is in use.


Refers to: src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:1676 in 34429cc. [](commit_id = 34429cc199d3e4a76b1cb0089cd3b818339577f8, deletion_comment = False)

@davidwrighton
Copy link
Member

            bool allowInstParam = (flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_ALLOWINSTPARAM) != 0;

After this initial setting of allowInstParam, you'll need to add the logic which will sometimes set the allowInstParam flag to false. Se PR #52173 for details.


Refers to: src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:1625 in 34429cc. [](commit_id = 34429cc199d3e4a76b1cb0089cd3b818339577f8, deletion_comment = False)

@davidwrighton
Copy link
Member

                        : DictionaryEntryKind.MethodEntrySlot);

Also, this will need to set useInstantiatingStub to false when we are performing a CORINFO_CALL_CODE_POINTER to a static method on a generic type.


In reply to: 859776971


Refers to: src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:1676 in 34429cc. [](commit_id = 34429cc199d3e4a76b1cb0089cd3b818339577f8, deletion_comment = False)

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

🕐

@trylek trylek force-pushed the Crossgen2SVMResolution branch 2 times, most recently from 2d70d77 to 0049f3d Compare June 20, 2021 20:02
Comment on lines 909 to 921
if (methodImpl.Decl.OwningType == interfaceType &&
methodImpl.Decl == interfaceMethod)
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to pass the interfaceType around? This condition can be simplified to if (methodImpl.Decl == interfaceMethod) because the other part is redundant.

The CoreCLR type system is weird and needs to pass around both the method and the owning type separately, but we abstract that weirdness away in CorInfoImpl and don't import it into the general type system.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, while I believe you're right that the outer method doesn't need the parameter, according to my understanding for TryResolveVirtualStaticMethodOnThisType we may need to match the method on a variance matched interface - perhaps that rather means that the method reference comparison is incorrect and we need to perform a structural comparison involving its name and signature?

@trylek trylek force-pushed the Crossgen2SVMResolution branch from 216b3a4 to 93940e8 Compare June 21, 2021 17:40
@MichalStrehovsky
Copy link
Member

I have removed the previously added special virtual method algorithm
for static virtual method resolution and instead I added logic for
SVM resolution to the existing methods

I meant that we don't have a bool flag for the normal interface method resolution - whether we can similarly have a pair of methods to do the static interface method resolution. It would let us have the same structure. I'm indifferent about piping it through the existing API surface as done in 93940e8.

{
directMethod = null;
}
forceUseRuntimeLookup = (directMethod == null);
Copy link
Member

Choose a reason for hiding this comment

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

If we return null because we can't find a result (as there isn't one), and we're not compiling shared generic code, we should produce a CodeGenerationFailedException. I know the current logic in coreclr doesn't fail in the obvious way here, but it does actually reliably fail, and we do have a testcase for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, why is it necessary to tear down compilation of the entire method? Wouldn't it be sufficient to just keep the constraint on a particular call and let it be resolved by the runtime?

/// </summary>
/// <param name="currentType">Type constraint in the static virtual method </param>
/// <param name="interfaceMethod">Static virtual interface method to resolve</param>
public bool AllowCompileTimeStaticVirtualMethodResolution(TypeDesc currentType, MethodDesc interfaceMethod)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be just AllowCompileTimeStaticVirtualMethodResolution, we should make the call to ResolveVariantInterfaceMethodToVirtualMethodOnType from within the DevirtualizationManager, as the full and compilex R2R version resilience rules will depend on the result of resolving the virtual call. In particular, it isn't required that the entire base type chain be within the bubble, but that the chain up until a result is found is within the version bubble.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, in such case it seems to me that we should rather include the version bubble edge logic directly in the recursive descent into base type in ResolveVirtualStaticMethod as in contrast to the runtime we only need to succeed the resolution attempt when we stay within the version bubble.

It also seems to me that we only need the variant form of the resolution as the only place where we're using the non-variant form in the runtime is class validity checks and we don't need to be doing that.

@trylek trylek force-pushed the Crossgen2SVMResolution branch from 1b76fb3 to be0a176 Compare June 30, 2021 13:18
@trylek trylek changed the title WIP: Crossgen2 support for static virtual method resolution Crossgen2 support for static virtual method resolution Jun 30, 2021
@trylek
Copy link
Member Author

trylek commented Jun 30, 2021

I have updated the change according to PR feedback and the SVM tests are now passing; I retried an infrastructural failure on Windows arm64 and I'm running the Crossgen2 composite tests to increase my confidence in the change. @davidwrighton / @MichalStrehovsky, can you please take another look when you have a chance?

Thanks

Tomas

@davidwrighton
Copy link
Member

@trylek When I run this logic against the GenericContextTest, in non-composite mode, I'm not pleased by the results. In particular if I run crossgen2 with the --verbose switch, one can see that in non-composite mode each and every method is not compiled, with a reason such as "Direct call to abstract method 'Void IFaceGeneric`1<System.String>.GenericMethod()' not allowed" This doesn't make sense to me. We should not be failing compilation for that reason, and I believe we should instead be generating a fixup to the constrained call. When I run the crossgen2 against TypeHierarchyTest, I see that many scenarios also report with a similar failure string. In that case, we don't have a version bubble to deal with, but we're still seeing compilation being deferred to runtime. That should not be happening.

@davidwrighton
Copy link
Member

The genericscontexttest now fails with an assertion when compiled with crossgen2 (non-composite mode). (Windows X64 Debug)

---------------------------
corerun.exe - Assert Failure (PID 36448, Thread 51492/0xc924)
---------------------------
decoderNew.ReadToken() == decoderExisting.ReadToken()

CORECLR! CheckGCRefMapEqual + 0x1A4 (0x00007ffd`88c8eef4)
CORECLR! ExternalMethodFixupWorker + 0xCD6 (0x00007ffd`88e0c666)
CORECLR! DelayLoad_MethodCall + 0x71 (0x00007ffd`8952f891)
GENERICCONTEXTTEST! <no symbol> + 0x0 (0x00007ffd`dec482fa)
GENERICCONTEXTTEST! <no symbol> + 0x0 (0x00007ffd`dec109d0)
CORECLR! CallDescrWorkerInternal + 0x83 (0x00007ffd`8952e513)
CORECLR! CallDescrWorkerWithHandler + 0x12E (0x00007ffd`88f3a24e)
CORECLR! MethodDescCallSite::CallTargetWorker + 0xCA4 (0x00007ffd`88f3af04)
CORECLR! MethodDescCallSite::Call_RetArgSlot + 0x11A (0x00007ffd`88ad606a)
CORECLR! RunMainInternal + 0x296 (0x00007ffd`88ae46a6)

C:\git2\runtime\src\coreclr\vm\frames.cpp, Line: 2084

Abort - Kill program
Retry - Debug
Ignore - Keep running


Image:
c:\git2\runtime\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\corerun.exe

---------------------------
Abort   Retry   Ignore   
---------------------------

/// <param name="interfaceMethod">Interface method to resolve</param>
/// <param name="currentType">Type to attempt virtual static method resolution on</param>
/// <returns>MethodDesc of the resolved virtual static method, null when not found (runtime lookup must be used)</returns>
public static MethodDesc ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType, Func<TypeDesc, bool> inVersionBubble)
Copy link
Member

Choose a reason for hiding this comment

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

The inVersionBubble parameter should probably go under #if READYTORUN.

We don't ifdef things in the type system, but also we don't put version bubble stuff here. So maybe an ifdef is less evil.

I do wonder why we were able to get by without a bool like this for the other virtual method resolution scenarios.

}

// Attempt to resolve on variance matched interface
MethodDesc runtimeInterfaceMethod = TryResolveInterfaceMethodOnVariantCompatibleInterface(runtimeInterfaceType, interfaceMethod);
Copy link
Member

Choose a reason for hiding this comment

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

I think we actually want to do this:

Suggested change
MethodDesc runtimeInterfaceMethod = TryResolveInterfaceMethodOnVariantCompatibleInterface(runtimeInterfaceType, interfaceMethod);
MethodDesc runtimeInterfaceMethod = runtimeInterfaceType.FindMethodOnExactTypeWithMatchingTypicalMethod(interfaceMethod);

// Attempt to resolve on variance matched interface
MethodDesc runtimeInterfaceMethod = TryResolveInterfaceMethodOnVariantCompatibleInterface(runtimeInterfaceType, interfaceMethod);

if (runtimeInterfaceMethod != null)
Copy link
Member

Choose a reason for hiding this comment

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

Under what condition could this be null?

return resolvedMethodOnType;
}

// Variant interface dispatch
Copy link
Member

Choose a reason for hiding this comment

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

The static virtual methods don't do a two pass variance check? (First try to find an exact match and only then do a second pass looking for variant match.)

If so could you please model these APIs the same as the existing "ResolveVariantInterfaceMethodToVirtualMethodOnType" and "ResolveInterfaceMethodToVirtualMethodOnType"? (i.e. add "ResolveInterfaceMethodToStaticVirtualMethodOnType" that "ResolveVariantInterfaceMethodToStaticVirtualMethodOnType" can call into as the first thing).

{
ThrowHelper.ThrowMissingMethodException(constrainedType, resolvedMethodImpl.Name, resolvedMethodImpl.Signature);
}
if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation)
if (interfaceMethod.HasInstantiation)

If interfaceMethod.HasInstantiation != methodImpl.Body.HasInstantiation, terrible things will happen below, so we might as well assume that and consider the Body check redundant.

I believe InstantiateSignature is going to be a no-op if the constrained type has an instantiation because that part is already instantiated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the latest commit, thanks for the suggestion.

}
if (interfaceMethod.HasInstantiation || methodImpl.Body.HasInstantiation || constrainedType.HasInstantiation)
{
resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resolvedMethodImpl = resolvedMethodImpl.InstantiateSignature(constrainedType.Instantiation, interfaceMethod.Instantiation);
resolvedMethodImpl = resolvedMethodImpl.MakeInstantiatedMethod(interfaceMethod.Instantiation);

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the latest commit, thanks for the suggestion.

{
foreach (MethodImplRecord methodImpl in mdType.FindMethodsImplWithMatchingDeclName(interfaceMethod.Name) ?? Array.Empty<MethodImplRecord>())
{
if (methodImpl.Decl == interfaceMethod)
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll need to compare with the interfaceMethod.GetMethodDefinition(). The code below assumes interfaceMethod is already instantiated if it's generic but the MethodImpl records you get from the constrained type will not be instantiated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the latest commit, thanks for explaining!

@trylek trylek force-pushed the Crossgen2SVMResolution branch from 516cf46 to 1c5207a Compare July 12, 2021 10:19
trylek added 6 commits July 19, 2021 17:28
This is my initial attempt at porting the CoreCLR runtime code
for SVM resolution to Crossgen2. The TypeHierarchyTest now fails
at 66-th scenario. The biggest problems I'm hitting are around
making sure that we drop the constraint from the actual entrypoint
import cells, otherwise we end up with constructs like

BaseScenario61.Method() @ DerivedScenario61

and that crashes the runtime. I guess that my treatment of generics
is most likely still incorrect.

Thanks

Tomas
I'm adding this as a separate commit as it's basically just a
projection of David's PR suggestions without additional algorithmic
changes. I'll follow up with additional fixes for the remaining
test failures.

Thanks

Tomas
I have removed the previously added special virtual method algorithm
for static virtual method resolution and instead I added logic for
SVM resolution to the existing methods

ResolveInterfaceMethodToVirtualMethodOnType

and

ResolveVariantInterfaceMethodToVirtualMethodOnType

I haven't made any functional changes in this commit.

Thanks

Tomas
trylek added 3 commits July 19, 2021 17:28
With this change, more tests seem to be passing but there seems to
be a runtime problem ending up as an assertion: for instance,
in TypeHierarchyTests, Scenario 25, TryResolveConstraintMethodApprox
resolves the constrained call to

BaseScenario25<Func<String>>.Method()

which subsequently gets wrapped into an instantiating stub and
later crashes GC ref map check which seems to compare the GC ref map
of the wrapped method with the original import cell for the
constrained method. These naturally don't match as the wrapped
canonical method requires the method dictionary argument. I continue
investigating this but any insight into what exactly needs fixing here
would be more than welcome.

Thanks

Tomas
@trylek trylek force-pushed the Crossgen2SVMResolution branch from 1c5207a to c3248ec Compare July 19, 2021 15:29
@kasperk81
Copy link
Contributor

@trylek, is svm support in crossgen2 planned for the next release?

@trylek
Copy link
Member Author

trylek commented Jan 24, 2022

@kasperk81 - yes, support for static virtual methods in Crossgen2 is part of our .NET Core 7 plans.

@tannergooding
Copy link
Member

@trylek, is there an ETA on this? Are we going to see failures in CI without this if we start merging PRs that utilize generic math in the BCL and marking it non-preview: #60905 (comment) ?

@trylek
Copy link
Member Author

trylek commented Feb 2, 2022

@tannergooding - I have yet to double-check with Manish and David but my gut feeling is that it would be good to get this in before Preview 4; later such a change becomes too risky due to insufficient bake time as it affects getCallInfo, one of the hottest JIT interface methods. Having said that, as I mentioned on the other thread, I'm still convinced that Crossgen2 support for SVMs is only a perf optimization - just like with HW intrinsics, the runtime is required to gracefully handle cases where certain methods aren't precompiled in the R2R native binary. For MonoAOT and NativeAOT the situation is different but explicit Crossgen2 SVM support is supposed to only handle (a) generating code for SVM overrides and (b) in some situations inlining SVM calls neither of which should be required for functional correctness.

@tannergooding
Copy link
Member

Thanks! I just wanted to double check as this was considered blocking other issues and preventing some work from going in.

We're trying to target Preview 2/3 for a lot of the other changes.

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Mar 2, 2022
Took the type system changes from dotnet#54063 and cleaned them up, added unit tests.

Hooked it up into JitInterface/ResolveConstraintMethodApprox. Using the pre-existing `ConstrainedMethodUseLookupResult` that wasn't currently getting emitted. We'll want to use it for its original purpose at some point, but I think we can make this work for both instance and static constrained calls.

Missing things:

* Support creating delegates to static virtual methods. This will need a RyuJIT/JitInterface change.
* Type loader support. If `MakeGeneric` needs static virtuals at runtime, it will throw.

But this is enough to get HttpClient working again. Fixes dotnet#65613. Contributes to dotnet/runtimelab#1665.
MichalStrehovsky added a commit that referenced this pull request Mar 3, 2022
Took the type system changes from #54063 and cleaned them up, added unit tests.

Hooked it up into JitInterface/ResolveConstraintMethodApprox. Using the pre-existing `ConstrainedMethodUseLookupResult` that wasn't currently getting emitted. We'll want to use it for its original purpose at some point, but I think we can make this work for both instance and static constrained calls.

Missing things:

* Support creating delegates to static virtual methods. This will need a RyuJIT/JitInterface change.
* Type loader support. If `MakeGeneric` needs static virtuals at runtime, it will throw.

But this is enough to get HttpClient working again. Fixes #65613. Contributes to dotnet/runtimelab#1665.
@jkotas
Copy link
Member

jkotas commented Jul 5, 2022

Stale PR

@jkotas jkotas closed this Jul 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 4, 2022
@trylek
Copy link
Member Author

trylek commented Jun 12, 2023

Apologies mostly to @MichalStrehovsky, I originally hoped to be able to revive this PR in order to keep all the PR feedback in place but apparently it's no longer there so I'll need to create a new one. I have rebased the change against current main and I believe I have addressed all Michal's feedback I'm aware of, I'm about to start testing the new version of this change shortly.

@trylek trylek modified the milestone: 8.0.0 Jun 12, 2023
@MichalStrehovsky
Copy link
Member

Apologies mostly to @MichalStrehovsky, I originally hoped to be able to revive this PR in order to keep all the PR feedback in place but apparently it's no longer there so I'll need to create a new one. I have rebased the change against current main and I believe I have addressed all Michal's feedback I'm aware of, I'm about to start testing the new version of this change shortly.

If you hit this in the future, I think you can recover this by clicking "Unlock conversation" on the PR (the link is at the bottom of the right column in the Github UI)

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