From 85382ee6dc880e5fe9bfa8a1dc6de909d3390504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 12 Dec 2016 17:08:27 -0800 Subject: [PATCH 1/3] Static methods on generic types use the type's generic dictionary Dependency analysis was incorrectly assuming these have their own dictionary (they don't). --- .../src/Compiler/DependencyAnalysis/GenericDictionaryNode.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericDictionaryNode.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericDictionaryNode.cs index 8b83b227a33..a66288e4ae2 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericDictionaryNode.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericDictionaryNode.cs @@ -112,8 +112,8 @@ public override IEnumerable GetConditionalStaticDep // that use the same dictionary layout. foreach (var method in _owningType.GetAllMethods()) { - // Static and generic methods have their own generic dictionaries - if (method.Signature.IsStatic || method.HasInstantiation) + // Generic methods have their own generic dictionaries + if (method.HasInstantiation) continue; // Abstract methods don't have a body From 9fa958488ca40ab0fe1e4e495961b7f802891862 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 12 Dec 2016 17:10:42 -0800 Subject: [PATCH 2/3] Emit R2R generic helpers for ldvirtftn --- .../DependencyAnalysis/GenericLookupResult.cs | 38 ++++++++++++ .../NodeFactory.GenericLookups.cs | 12 ++++ .../ReadyToRunGenericHelperNode.cs | 2 + .../X64ReadyToRunGenericHelperNode.cs | 1 + src/JitInterface/src/CorInfoImpl.cs | 42 ++++++++----- tests/src/Simple/Generics/Generics.cs | 59 +++++++++++++++++++ 6 files changed, 138 insertions(+), 16 deletions(-) diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs index 76a470d8c62..0be92a1197d 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs @@ -128,6 +128,11 @@ internal sealed class VirtualDispatchGenericLookupResult : GenericLookupResult public VirtualDispatchGenericLookupResult(MethodDesc method) { Debug.Assert(method.IsRuntimeDeterminedExactMethod); + Debug.Assert(method.IsVirtual); + + // Normal virtual methods don't need a generic lookup. + Debug.Assert(method.OwningType.IsInterface || method.HasInstantiation); + _method = method; } @@ -146,6 +151,39 @@ public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilde public override string ToString() => $"VirtualCall: {_method}"; } + /// + /// Generic lookup result that points to a virtual function address load stub. + /// + internal sealed class VirtualResolveGenericLookupResult : GenericLookupResult + { + private MethodDesc _method; + + public VirtualResolveGenericLookupResult(MethodDesc method) + { + Debug.Assert(method.IsRuntimeDeterminedExactMethod); + Debug.Assert(method.IsVirtual); + + // Normal virtual methods don't need a generic lookup. + Debug.Assert(method.OwningType.IsInterface || method.HasInstantiation); + + _method = method; + } + + public override ISymbolNode GetTarget(NodeFactory factory, Instantiation typeInstantiation, Instantiation methodInstantiation) + { + MethodDesc instantiatedMethod = _method.InstantiateSignature(typeInstantiation, methodInstantiation); + return factory.ReadyToRunHelper(ReadyToRunHelperId.VirtualCall, instantiatedMethod); + } + + public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) + { + sb.Append("VirtualResolve_"); + sb.Append(nameMangler.GetMangledMethodName(_method)); + } + + public override string ToString() => $"VirtualResolve: {_method}"; + } + /// /// Generic lookup result that points to the non-GC static base of a type. /// diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs index 36a227f487f..decf899d185 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs @@ -42,6 +42,11 @@ private void CreateNodeCaches() return new VirtualDispatchGenericLookupResult(method); }); + _virtualResolveHelpers = new NodeCache(method => + { + return new VirtualResolveGenericLookupResult(method); + }); + _typeGCStaticBaseSymbols = new NodeCache(type => { return new TypeGCStaticBaseGenericLookupResult(type); @@ -88,6 +93,13 @@ public GenericLookupResult VirtualCall(MethodDesc method) return _virtualCallHelpers.GetOrAdd(method); } + private NodeCache _virtualResolveHelpers; + + public GenericLookupResult VirtualMethodAddress(MethodDesc method) + { + return _virtualResolveHelpers.GetOrAdd(method); + } + private NodeCache _methodEntrypoints; public GenericLookupResult MethodEntry(MethodDesc method) diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs index 98e36e587c8..e6885423582 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs @@ -40,6 +40,8 @@ private static GenericLookupResult GetLookupSignature(NodeFactory factory, Ready return factory.GenericLookup.MethodDictionary((MethodDesc)target); case ReadyToRunHelperId.VirtualCall: return factory.GenericLookup.VirtualCall((MethodDesc)target); + case ReadyToRunHelperId.ResolveVirtualFunction: + return factory.GenericLookup.VirtualMethodAddress((MethodDesc)target); case ReadyToRunHelperId.MethodEntry: return factory.GenericLookup.MethodEntry((MethodDesc)target); default: diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs index b3859b87bb9..9b5bba78bd2 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs @@ -103,6 +103,7 @@ protected sealed override void EmitCode(NodeFactory factory, ref X64Emitter enco case ReadyToRunHelperId.TypeHandle: case ReadyToRunHelperId.MethodDictionary: case ReadyToRunHelperId.VirtualCall: + case ReadyToRunHelperId.ResolveVirtualFunction: case ReadyToRunHelperId.MethodEntry: { EmitDictionaryLookup(factory, ref encoder, encoder.TargetRegister.Arg0, encoder.TargetRegister.Result, _lookupSignature, relocsOnly); diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs index 2f4e6606811..f899e8c5543 100644 --- a/src/JitInterface/src/CorInfoImpl.cs +++ b/src/JitInterface/src/CorInfoImpl.cs @@ -2683,23 +2683,33 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(ReadyToRunHelperId.ResolveGenericVirtualMethod, targetMethod)); pResult.nullInstanceCheck = false; } - else if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) + else { - pResult.kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN; - pResult.codePointerOrStubLookup.constLookup.accessType = InfoAccessType.IAT_VALUE; + ReadyToRunHelperId helper; - pResult.codePointerOrStubLookup.constLookup.addr = - (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(ReadyToRunHelperId.ResolveVirtualFunction, targetMethod)); + if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) + { + pResult.kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN; + helper = ReadyToRunHelperId.ResolveVirtualFunction; + } + else + { + // CORINFO_CALL_CODE_POINTER tells the JIT that this is indirect + // call that should not be inlined. + pResult.kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER; + helper = ReadyToRunHelperId.VirtualCall; + } - // The current CoreRT ReadyToRun helpers do not handle null thisptr - ask the JIT to emit explicit null checks - // TODO: Optimize this - pResult.nullInstanceCheck = true; - } - else - { - // CORINFO_CALL_CODE_POINTER tells the JIT that this is indirect - // call that should not be inlined. - pResult.kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER; + // If this is a non-interface/non-GVM call, we actually don't need a runtime lookup to find the target. + // We don't even need to keep track of the runtime-determined method being called because the system ensures + // that if e.g. Foo<__Canon>.GetHashCode is needed and we're generating a dictionary for Foo, + // Foo.GetHashCode is needed too. + if (pResult.exactContextNeedsRuntimeLookup && + !targetMethod.HasInstantiation && + !targetMethod.OwningType.IsInterface) + { + pResult.exactContextNeedsRuntimeLookup = false; + } if (pResult.exactContextNeedsRuntimeLookup) { @@ -2716,14 +2726,14 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO } pResult.codePointerOrStubLookup.lookupKind.runtimeLookupKind = GetGenericRuntimeLookupKind(contextMethod); - pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)ReadyToRunHelperId.VirtualCall; + pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)helper; } else { pResult.codePointerOrStubLookup.constLookup.accessType = InfoAccessType.IAT_VALUE; pResult.codePointerOrStubLookup.constLookup.addr = - (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(ReadyToRunHelperId.VirtualCall, targetMethod)); + (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(helper, targetMethod)); } // The current CoreRT ReadyToRun helpers do not handle null thisptr - ask the JIT to emit explicit null checks diff --git a/tests/src/Simple/Generics/Generics.cs b/tests/src/Simple/Generics/Generics.cs index 859a6b98ca9..d181130ac66 100644 --- a/tests/src/Simple/Generics/Generics.cs +++ b/tests/src/Simple/Generics/Generics.cs @@ -14,6 +14,8 @@ static int Main() TestDelegateFatFunctionPointers.Run(); TestVirtualMethodUseTracking.Run(); TestSlotsInHierarchy.Run(); + TestDelegateVirtualMethod.Run(); + TestDelegateInterfaceMethod.Run(); TestNameManglingCollisionRegression.Run(); TestUnusedGVMsDoNotCrashCompiler.Run(); @@ -142,6 +144,63 @@ public static void Run() } } + class TestDelegateVirtualMethod + { + static void Generic() + { + Base o = new Derived(); + Func f = o.Do; + if (f() != "Derived") + throw new Exception(); + + o = new Base(); + f = o.Do; + if (f() != "Base") + throw new Exception(); + } + + public static void Run() + { + Generic(); + } + + class Base + { + public virtual string Do() => "Base"; + } + + class Derived : Base + { + public override string Do() => "Derived"; + } + } + + class TestDelegateInterfaceMethod + { + static void Generic() + { + IFoo o = new Foo(); + Func f = o.Do; + if (f() != "Foo") + throw new Exception(); + } + + public static void Run() + { + Generic(); + } + + interface IFoo + { + string Do(); + } + + class Foo : IFoo + { + public string Do() => "Foo"; + } + } + /// /// Tests RyuJIT's initThisClass. /// From 77514ad299baf65459d4f26d0647bea6b6b4e47c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 14 Dec 2016 13:32:06 -0800 Subject: [PATCH 3/3] CR feedback --- .../DependencyAnalysis/GenericLookupResult.cs | 3 + src/JitInterface/src/CorInfoImpl.cs | 59 +++++++++++++------ 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs index 0be92a1197d..5760d328bb0 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs @@ -172,6 +172,9 @@ public VirtualResolveGenericLookupResult(MethodDesc method) public override ISymbolNode GetTarget(NodeFactory factory, Instantiation typeInstantiation, Instantiation methodInstantiation) { MethodDesc instantiatedMethod = _method.InstantiateSignature(typeInstantiation, methodInstantiation); + + // https://github.com/dotnet/corert/issues/2342 - we put a pointer to the virtual call helper into the dictionary + // but this should be something that will let us compute the target of the dipatch (e.g. interface dispatch cell). return factory.ReadyToRunHelper(ReadyToRunHelperId.VirtualCall, instantiatedMethod); } diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs index f899e8c5543..99ccaad4eb3 100644 --- a/src/JitInterface/src/CorInfoImpl.cs +++ b/src/JitInterface/src/CorInfoImpl.cs @@ -2683,38 +2683,58 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(ReadyToRunHelperId.ResolveGenericVirtualMethod, targetMethod)); pResult.nullInstanceCheck = false; } - else + else if((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) { - ReadyToRunHelperId helper; + pResult.kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN; - if ((flags & CORINFO_CALLINFO_FLAGS.CORINFO_CALLINFO_LDFTN) != 0) + // If this is a non-interface/non-GVM call, we actually don't need a runtime lookup to find the target. + // We don't even need to keep track of the runtime-determined method being called because the system ensures + // that if e.g. Foo<__Canon>.GetHashCode is needed and we're generating a dictionary for Foo, + // Foo.GetHashCode is needed too. + if (pResult.exactContextNeedsRuntimeLookup && + (targetMethod.HasInstantiation || targetMethod.OwningType.IsInterface)) { - pResult.kind = CORINFO_CALL_KIND.CORINFO_VIRTUALCALL_LDVIRTFTN; - helper = ReadyToRunHelperId.ResolveVirtualFunction; + pResult.codePointerOrStubLookup.lookupKind.needsRuntimeLookup = true; + pResult.codePointerOrStubLookup.runtimeLookup.indirections = CORINFO.USEHELPER; + + // Do not bother computing the runtime lookup if we are inlining. The JIT is going + // to abort the inlining attempt anyway. + MethodDesc contextMethod = methodFromContext(pResolvedToken.tokenContext); + if (contextMethod != MethodBeingCompiled) + { + return; + } + + pResult.codePointerOrStubLookup.lookupKind.runtimeLookupKind = GetGenericRuntimeLookupKind(contextMethod); + pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)ReadyToRunHelperId.ResolveVirtualFunction; } else { - // CORINFO_CALL_CODE_POINTER tells the JIT that this is indirect - // call that should not be inlined. - pResult.kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER; - helper = ReadyToRunHelperId.VirtualCall; + pResult.exactContextNeedsRuntimeLookup = false; + pResult.codePointerOrStubLookup.constLookup.accessType = InfoAccessType.IAT_VALUE; + + pResult.codePointerOrStubLookup.constLookup.addr = + (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(ReadyToRunHelperId.ResolveVirtualFunction, targetMethod)); } + // The current CoreRT ReadyToRun helpers do not handle null thisptr - ask the JIT to emit explicit null checks + // TODO: Optimize this + pResult.nullInstanceCheck = true; + } + else + { + // CORINFO_CALL_CODE_POINTER tells the JIT that this is indirect + // call that should not be inlined. + pResult.kind = CORINFO_CALL_KIND.CORINFO_CALL_CODE_POINTER; + // If this is a non-interface/non-GVM call, we actually don't need a runtime lookup to find the target. // We don't even need to keep track of the runtime-determined method being called because the system ensures // that if e.g. Foo<__Canon>.GetHashCode is needed and we're generating a dictionary for Foo, // Foo.GetHashCode is needed too. if (pResult.exactContextNeedsRuntimeLookup && - !targetMethod.HasInstantiation && - !targetMethod.OwningType.IsInterface) - { - pResult.exactContextNeedsRuntimeLookup = false; - } - - if (pResult.exactContextNeedsRuntimeLookup) + (targetMethod.HasInstantiation || targetMethod.OwningType.IsInterface)) { pResult.codePointerOrStubLookup.lookupKind.needsRuntimeLookup = true; - pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = 0; pResult.codePointerOrStubLookup.runtimeLookup.indirections = CORINFO.USEHELPER; // Do not bother computing the runtime lookup if we are inlining. The JIT is going @@ -2726,14 +2746,15 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO } pResult.codePointerOrStubLookup.lookupKind.runtimeLookupKind = GetGenericRuntimeLookupKind(contextMethod); - pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)helper; + pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)ReadyToRunHelperId.VirtualCall; } else { + pResult.exactContextNeedsRuntimeLookup = false; pResult.codePointerOrStubLookup.constLookup.accessType = InfoAccessType.IAT_VALUE; pResult.codePointerOrStubLookup.constLookup.addr = - (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(helper, targetMethod)); + (void*)ObjectToHandle(_compilation.NodeFactory.ReadyToRunHelper(ReadyToRunHelperId.VirtualCall, targetMethod)); } // The current CoreRT ReadyToRun helpers do not handle null thisptr - ask the JIT to emit explicit null checks