Cleanups after enabling virtual method inlining (#3867)

Pull request #3773 enabled some cleanups I didn't want to do there
because it's a rather big diff with a possible (expected) test
regression:

* ReadyToRunHelperId.VirtualCall and
ReadyToRunHelperId.ResolveVirtualFunction should no longer show up in
connection with generic lookups. We only used them for interface calls
and for those, RyuJIT now knows how to handle with dispatch cells
directly. They're still used in non-generic codepaths where they handle
normal virtual calls with unknown slot numbers.
* Standalone LDVIRTFTN for interface methods on generic interfaces used
a hack where we placed a R2R helper into the dictionary and returned
that. Given this is only reachable outside of delegate creation, I'm
deleting the hack. If we need it, we should implement it the right way,
because this returns function pointers to things that are not user code.
It works most of the time, but not always. I'm expecting some rolling
build fallout from this.
* I'm deleting one of the hacky generic dictionary entries. We should
rename the second one, but that's best done from TFS.

7 files changed, 15 insertions, 145 deletions

diff --git a/src/ILCompiler.Compiler/src/Compiler/DelegateCreationInfo.cs b/src/ILCompiler.Compiler/src/Compiler/DelegateCreationInfo.cs
index eb69b1146..005af801a 100644
--- a/src/ILCompiler.Compiler/src/Compiler/DelegateCreationInfo.cs
+++ b/src/ILCompiler.Compiler/src/Compiler/DelegateCreationInfo.cs
@@ -96,7 +96,7 @@ namespace ILCompiler
                     return factory.GenericLookup.MethodEntry(TargetMethod, TargetMethodIsUnboxingThunk);
 
                 case TargetKind.InterfaceDispatch:
-                    return factory.GenericLookup.VirtualMethodAddress(TargetMethod);
+                    return factory.GenericLookup.VirtualCall(TargetMethod);
 
                 case TargetKind.MethodHandle:
                     return factory.GenericLookup.MethodHandle(TargetMethod);
diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs
index 05d2bb9e7..6b8765c10 100644
--- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs
+++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GenericLookupResult.cs
@@ -610,7 +610,7 @@ namespace ILCompiler.DependencyAnalysis
             if (factory.Target.Abi == TargetAbi.CoreRT)
             {
                 MethodDesc instantiatedMethod = _method.GetNonRuntimeDeterminedMethodFromRuntimeDeterminedMethodViaSubstitution(typeInstantiation, methodInstantiation);
-                return factory.ReadyToRunHelper(ReadyToRunHelperId.VirtualCall, instantiatedMethod);
+                return factory.InterfaceDispatchCell(instantiatedMethod);
             }
             else
             {
@@ -629,27 +629,12 @@ namespace ILCompiler.DependencyAnalysis
 
         public override NativeLayoutVertexNode TemplateDictionaryNode(NodeFactory factory)
         {
-            if (factory.Target.Abi == TargetAbi.CoreRT)
-            {
-                return factory.NativeLayout.NotSupportedDictionarySlot;
-            }
-            else
-            {
-                return factory.NativeLayout.InterfaceCellDictionarySlot(_method);
-            }
+            return factory.NativeLayout.InterfaceCellDictionarySlot(_method);
         }
 
         public override void WriteDictionaryTocData(NodeFactory factory, IGenericLookupResultTocWriter writer)
         {
-            if (factory.Target.Abi == TargetAbi.CoreRT)
-            {
-                // TODO
-                throw new NotImplementedException();
-            }
-            else
-            {
-                writer.WriteData(LookupResultReferenceType(factory), LookupResultType.InterfaceDispatchCell, _method);
-            }
+            writer.WriteData(LookupResultReferenceType(factory), LookupResultType.InterfaceDispatchCell, _method);
         }
 
         protected override int CompareToImpl(GenericLookupResult other, TypeSystemComparer comparer)
@@ -659,81 +644,6 @@ namespace ILCompiler.DependencyAnalysis
     }
 
     /// <summary>
-    /// Generic lookup result that points to a virtual function address load stub.
-    /// </summary>
-    internal sealed class VirtualResolveGenericLookupResult : GenericLookupResult
-    {
-        private MethodDesc _method;
-
-        protected override int ClassCode => -12619218;
-
-        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, GenericDictionaryNode dictionary)
-        {
-            if (factory.Target.Abi == TargetAbi.CoreRT)
-            {
-                MethodDesc instantiatedMethod = _method.GetNonRuntimeDeterminedMethodFromRuntimeDeterminedMethodViaSubstitution(typeInstantiation, methodInstantiation);
-                return factory.InterfaceDispatchCell(instantiatedMethod);
-            }
-            else
-            {
-                MethodDesc instantiatedMethod = _method.GetNonRuntimeDeterminedMethodFromRuntimeDeterminedMethodViaSubstitution(dictionary.TypeInstantiation, dictionary.MethodInstantiation);
-                return factory.InterfaceDispatchCell(instantiatedMethod, dictionary.GetMangledName(factory.NameMangler));
-            }
-        }
-
-        public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
-        {
-            sb.Append("VirtualResolve_");
-            sb.Append(nameMangler.GetMangledMethodName(_method));
-        }
-
-        public override string ToString() => $"VirtualResolve: {_method}";
-
-        public override NativeLayoutVertexNode TemplateDictionaryNode(NodeFactory factory)
-        {
-            if (factory.Target.Abi == TargetAbi.CoreRT)
-            {
-                // We should be able to get rid of this custom ABI handling
-                // once https://github.com/dotnet/corert/issues/3248 is fixed.
-                return factory.NativeLayout.NotSupportedDictionarySlot;
-            }
-            else
-            {
-                return factory.NativeLayout.InterfaceCellDictionarySlot(_method);
-            }
-        }
-
-        public override void WriteDictionaryTocData(NodeFactory factory, IGenericLookupResultTocWriter writer)
-        {
-            if (factory.Target.Abi == TargetAbi.CoreRT)
-            {
-                // TODO
-                throw new NotImplementedException();
-            }
-            else
-            {
-                writer.WriteData(LookupResultReferenceType(factory), LookupResultType.InterfaceDispatchCell, _method);
-            }
-        }
-
-        protected override int CompareToImpl(GenericLookupResult other, TypeSystemComparer comparer)
-        {
-            return comparer.Compare(_method, ((VirtualResolveGenericLookupResult)other)._method);
-        }
-    }
-
-    /// <summary>
     /// Generic lookup result that points to the non-GC static base of a type.
     /// </summary>
     internal sealed class TypeNonGCStaticBaseGenericLookupResult : GenericLookupResult
diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs
index 3ec7d5fd2..c8f936604 100644
--- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs
+++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs
@@ -1583,7 +1583,10 @@ namespace ILCompiler.DependencyAnalysis
         protected sealed override FixupSignatureKind SignatureKind => FixupSignatureKind.InterfaceCall;
         public sealed override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory)
         {
-            return new DependencyListEntry[1] { new DependencyListEntry(_signature, "TypeSignature") };
+            yield return new DependencyListEntry(_signature, "TypeSignature");
+
+            if (!factory.VTable(_method.OwningType).HasFixedSlots)
+                yield return new DependencyListEntry(factory.VirtualMethodUse(_method), "Slot number");
         }
 
         protected sealed override Vertex WriteSignatureVertex(NativeWriter writer, NodeFactory factory)
diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs
index 73bb229d8..ba32b6a37 100644
--- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs
+++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.GenericLookups.cs
@@ -58,11 +58,6 @@ namespace ILCompiler.DependencyAnalysis
                     return new VirtualDispatchGenericLookupResult(method);
                 });
 
-                _virtualResolveHelpers = new NodeCache<MethodDesc, GenericLookupResult>(method =>
-                {
-                    return new VirtualResolveGenericLookupResult(method);
-                });
-
                 _typeThreadStaticBaseIndexSymbols = new NodeCache<TypeDesc, GenericLookupResult>(type =>
                 {
                     return new TypeThreadStaticBaseIndexGenericLookupResult(type);
@@ -213,13 +208,6 @@ namespace ILCompiler.DependencyAnalysis
                 return _virtualCallHelpers.GetOrAdd(method);
             }
 
-            private NodeCache<MethodDesc, GenericLookupResult> _virtualResolveHelpers;
-
-            public GenericLookupResult VirtualMethodAddress(MethodDesc method)
-            {
-                return _virtualResolveHelpers.GetOrAdd(method);
-            }
-
             private NodeCache<MethodKey, GenericLookupResult> _methodEntrypoints;
 
             public GenericLookupResult MethodEntry(MethodDesc method, bool isUnboxingThunk = false)
diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
index 6f91ff373..e88762751 100644
--- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
+++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ReadyToRunGenericHelperNode.cs
@@ -47,12 +47,8 @@ namespace ILCompiler.DependencyAnalysis
                     return factory.GenericLookup.TypeThreadStaticBaseIndex((TypeDesc)target);
                 case ReadyToRunHelperId.MethodDictionary:
                     return factory.GenericLookup.MethodDictionary((MethodDesc)target);
-                case ReadyToRunHelperId.VirtualCall:
-                    return factory.GenericLookup.VirtualCall((MethodDesc)target);
                 case ReadyToRunHelperId.VirtualDispatchCell:
-                    return factory.GenericLookup.VirtualMethodAddress((MethodDesc)target);
-                case ReadyToRunHelperId.ResolveVirtualFunction:
-                    return factory.GenericLookup.VirtualMethodAddress((MethodDesc)target);
+                    return factory.GenericLookup.VirtualCall((MethodDesc)target);
                 case ReadyToRunHelperId.MethodEntry:
                     return factory.GenericLookup.MethodEntry((MethodDesc)target);
                 case ReadyToRunHelperId.DelegateCtor:
@@ -131,25 +127,6 @@ namespace ILCompiler.DependencyAnalysis
                         }
                     }
                     break;
-
-                case ReadyToRunHelperId.ResolveVirtualFunction:
-                    {
-                        MethodDesc instantiatedTarget = ((MethodDesc)_target).GetNonRuntimeDeterminedMethodFromRuntimeDeterminedMethodViaSubstitution(typeInstantiation, methodInstantiation);
-                        if (!factory.VTable(instantiatedTarget.OwningType).HasFixedSlots)
-                        {
-                            result.Add(
-                                new DependencyListEntry(
-                                    factory.VirtualMethodUse(instantiatedTarget),
-                                    "Dictionary dependency"));
-                        }
-
-                        // TODO: https://github.com/dotnet/corert/issues/3224 
-                        if (instantiatedTarget.IsAbstract)
-                        {
-                            result.Add(new DependencyListEntry(factory.ReflectableMethod(instantiatedTarget), "Abstract reflectable method"));
-                        }
-                    }
-                    break;
             }
 
             // All generic lookups depend on the thing they point to
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 e3b4ac7b9..fca8037df 100644
--- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs
+++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunGenericHelperNode.cs
@@ -207,8 +207,6 @@ namespace ILCompiler.DependencyAnalysis
                 case ReadyToRunHelperId.MethodHandle:
                 case ReadyToRunHelperId.FieldHandle:
                 case ReadyToRunHelperId.MethodDictionary:
-                case ReadyToRunHelperId.VirtualCall:
-                case ReadyToRunHelperId.ResolveVirtualFunction:
                 case ReadyToRunHelperId.MethodEntry:
                 case ReadyToRunHelperId.VirtualDispatchCell:
                 case ReadyToRunHelperId.DefaultConstructor:
diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs
index a13855c08..9c8534db5 100644
--- a/src/JitInterface/src/CorInfoImpl.cs
+++ b/src/JitInterface/src/CorInfoImpl.cs
@@ -3130,18 +3130,12 @@ namespace Internal.JitInterface
                 // Foo<string>.GetHashCode is needed too.
                 if (pResult.exactContextNeedsRuntimeLookup && targetMethod.OwningType.IsInterface)
                 {
-                    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)
-                    {
-                        pResult.codePointerOrStubLookup.lookupKind.runtimeLookupKind = GetGenericRuntimeLookupKind(contextMethod);
-                        pResult.codePointerOrStubLookup.lookupKind.runtimeLookupFlags = (ushort)helperId;
-                        pResult.codePointerOrStubLookup.lookupKind.runtimeLookupArgs = (void*)ObjectToHandle(GetRuntimeDeterminedObjectForToken(ref pResolvedToken));
-                    }
+                    // We need JitInterface changes to fully support this.
+                    // If this is LDVIRTFTN of an interface method that is part of a verifiable delegate creation sequence,
+                    // RyuJIT is not going to use this value.
+                    Debug.Assert(helperId == ReadyToRunHelperId.ResolveVirtualFunction);
+                    pResult.exactContextNeedsRuntimeLookup = false;
+                    pResult.codePointerOrStubLookup.constLookup = CreateConstLookupToSymbol(_compilation.NodeFactory.ExternSymbol("NYI_LDVIRTFTN"));
                 }
                 else
                 {