Hide constructed/necessary type duality from RyuJIT (#1754)

If we have whole program view, use that to upgrade requests for "necessary" types to "constructed" types if we know a constructed was seen during scanning. This avoids RyuJIT seeing a constructed and necessary symbol for the same type.

If we don't have whole program view, forgo the optimization and use maximally loadable types instead of necessary types.

src/coreclr/jit/morph.cpp

@@ -9408,14 +9408,14 @@ GenTree* Compiler::fgExpandVirtualVtableCallTarget(GenTreeCall* call)
 
             // [tmp + vtabOffsOfIndirection]
             GenTree* tmpTree1 = gtNewOperNode(GT_ADD, TYP_I_IMPL, gtNewLclvNode(varNum1, TYP_I_IMPL),
-                                              gtNewIconNode(vtabOffsOfIndirection, TYP_INT));
+                                              gtNewIconNode(vtabOffsOfIndirection, TYP_I_IMPL));
             tmpTree1 = gtNewOperNode(GT_IND, TYP_I_IMPL, tmpTree1, false);
             tmpTree1->gtFlags |= GTF_IND_NONFAULTING;
             tmpTree1->gtFlags |= GTF_IND_INVARIANT;
 
             // var1 + vtabOffsOfIndirection + vtabOffsAfterIndirection
             GenTree* tmpTree2 = gtNewOperNode(GT_ADD, TYP_I_IMPL, gtNewLclvNode(varNum1, TYP_I_IMPL),
-                                              gtNewIconNode(vtabOffsOfIndirection + vtabOffsAfterIndirection, TYP_INT));
+                                              gtNewIconNode(vtabOffsOfIndirection + vtabOffsAfterIndirection, TYP_I_IMPL));
 
             // var1 + vtabOffsOfIndirection + vtabOffsAfterIndirection + [var1 + vtabOffsOfIndirection]
             tmpTree2         = gtNewOperNode(GT_ADD, TYP_I_IMPL, tmpTree2, tmpTree1);
@@ -9434,7 +9434,7 @@ GenTree* Compiler::fgExpandVirtualVtableCallTarget(GenTreeCall* call)
         else
         {
             // result = [vtab + vtabOffsOfIndirection]
-            result = gtNewOperNode(GT_ADD, TYP_I_IMPL, vtab, gtNewIconNode(vtabOffsOfIndirection, TYP_INT));
+            result = gtNewOperNode(GT_ADD, TYP_I_IMPL, vtab, gtNewIconNode(vtabOffsOfIndirection, TYP_I_IMPL));
             result = gtNewOperNode(GT_IND, TYP_I_IMPL, result, false);
             result->gtFlags |= GTF_IND_NONFAULTING;
             result->gtFlags |= GTF_IND_INVARIANT;
@@ -9450,7 +9450,7 @@ GenTree* Compiler::fgExpandVirtualVtableCallTarget(GenTreeCall* call)
     {
         // Load the function address
         // result = [result + vtabOffsAfterIndirection]
-        result = gtNewOperNode(GT_ADD, TYP_I_IMPL, result, gtNewIconNode(vtabOffsAfterIndirection, TYP_INT));
+        result = gtNewOperNode(GT_ADD, TYP_I_IMPL, result, gtNewIconNode(vtabOffsAfterIndirection, TYP_I_IMPL));
         // This last indirection is not invariant, but is non-faulting
         result = gtNewOperNode(GT_IND, TYP_I_IMPL, result, false);
         result->gtFlags |= GTF_IND_NONFAULTING;

src/coreclr/jit/valuenum.cpp

@@ -8815,7 +8815,6 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                 // Are we dereferencing the method table slot of some newly allocated object?
                 //
                 bool wasNewobj = false;
-#if 0 // TODO: https://github.com/dotnet/runtimelab/issues/1128
                 if ((oper == GT_IND) && (addr->TypeGet() == TYP_REF) && (tree->TypeGet() == TYP_I_IMPL))
                 {
                     VNFuncApp  funcApp;
@@ -8828,7 +8827,6 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                         wasNewobj = true;
                     }
                 }
-#endif
 
                 if (!wasNewobj)
                 {

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Compilation.cs

@@ -26,7 +26,7 @@ public abstract class Compilation : ICompilation
         protected readonly NodeFactory _nodeFactory;
         protected readonly Logger _logger;
         protected readonly DebugInformationProvider _debugInformationProvider;
-        private readonly DevirtualizationManager _devirtualizationManager;
+        protected readonly DevirtualizationManager _devirtualizationManager;
         private readonly IInliningPolicy _inliningPolicy;
 
         public NameMangler NameMangler => _nodeFactory.NameMangler;
@@ -97,6 +97,11 @@ public void DetectGenericCycles(MethodDesc caller, MethodDesc callee)
             _nodeFactory.TypeSystemContext.DetectGenericCycles(caller, callee);
         }
 
+        public virtual IEETypeNode NecessaryTypeSymbolIfPossible(TypeDesc type)
+        {
+            return _nodeFactory.NecessaryTypeSymbol(type);
+        }
+
         public bool CanInline(MethodDesc caller, MethodDesc callee)
         {
             return _inliningPolicy.CanInline(caller, callee);
@@ -285,13 +290,13 @@ public ISymbolNode ComputeConstantLookup(ReadyToRunHelperId lookupKind, object t
                 case ReadyToRunHelperId.TypeHandle:
                     return NodeFactory.ConstructedTypeSymbol((TypeDesc)targetOfLookup);
                 case ReadyToRunHelperId.NecessaryTypeHandle:
-                    return NodeFactory.NecessaryTypeSymbol((TypeDesc)targetOfLookup);
+                    return NecessaryTypeSymbolIfPossible((TypeDesc)targetOfLookup);
                 case ReadyToRunHelperId.TypeHandleForCasting:
                     {
                         var type = (TypeDesc)targetOfLookup;
                         if (type.IsNullable)
                             targetOfLookup = type.Instantiation[0];
-                        return NodeFactory.NecessaryTypeSymbol((TypeDesc)targetOfLookup);
+                        return NecessaryTypeSymbolIfPossible((TypeDesc)targetOfLookup);
                     }
                 case ReadyToRunHelperId.MethodDictionary:
                     return NodeFactory.MethodGenericDictionary((MethodDesc)targetOfLookup);

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs

@@ -405,14 +405,9 @@ public ScannedDevirtualizationManager(ImmutableArray<DependencyNodeCore<NodeFact
                             //    didn't scan the virtual methods on them.
                             //
 
-                            TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);
-
-                            _constructedTypes.Add(canonType);
+                            _constructedTypes.Add(type);
 
-                            // Since this is used for the purposes of devirtualization, it's really convenient
-                            // to also have Array<T> for each T[].
-                            if (canonType.IsArray)
-                                _constructedTypes.Add(canonType.GetClosestDefType());
+                            TypeDesc canonType = type.ConvertToCanonForm(CanonicalFormKind.Specific);
 
                             bool hasNonAbstractTypeInHierarchy = canonType is not MetadataType mdType || !mdType.IsAbstract;
                             TypeDesc baseType = canonType.BaseType;

src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilation.cs

@@ -63,6 +63,23 @@ internal RyuJitCompilation(
 
         public ProfileDataManager ProfileData => _profileDataManager;
 
+        public override IEETypeNode NecessaryTypeSymbolIfPossible(TypeDesc type)
+        {
+            // RyuJIT makes assumptions around the value of these symbols - in particular, it assumes
+            // that type handles and type symbols have a 1:1 relationship. We therefore need to
+            // make sure RyuJIT never sees a constructed and unconstructed type symbol for the
+            // same type. If the type is constructable and we don't have whole progam view
+            // information proving that it isn't, give RyuJIT the constructed symbol even
+            // though we just need the unconstructed one.
+            // https://github.com/dotnet/runtimelab/issues/1128
+            bool canPotentiallyConstruct = _devirtualizationManager == null
+                ? true : _devirtualizationManager.CanConstructType(type);
+            if (canPotentiallyConstruct)
+                return _nodeFactory.MaximallyConstructableType(type);
+
+            return _nodeFactory.NecessaryTypeSymbol(type);
+        }
+
         protected override void CompileInternal(string outputFile, ObjectDumper dumper)
         {
             _dependencyGraph.ComputeMarkedNodes();

src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs

@@ -63,6 +63,20 @@ public void CompileMethod(MethodCodeNode methodCodeNodeNeedingCode, MethodIL met
             }
             finally
             {
+#if DEBUG
+                // RyuJIT makes assumptions around the value of type symbols - in particular, it assumes
+                // that type handles and type symbols have a 1:1 relationship. We therefore need to
+                // make sure RyuJIT never sees a constructed and unconstructed type symbol for the
+                // same type. This check makes sure we didn't accidentally hand out a necessary type symbol
+                // that the compilation class didn't agree to handing out.
+                // https://github.com/dotnet/runtimelab/issues/1128
+                for (int i = 0; i < _codeRelocs.Count; i++)
+                {
+                    Debug.Assert(_codeRelocs[i].Target.GetType() != typeof(EETypeNode)
+                        || _compilation.NecessaryTypeSymbolIfPossible(((EETypeNode)_codeRelocs[i].Target).Type) == _codeRelocs[i].Target);
+                }
+#endif
+
                 CompileMethodCleanup();
             }
         }
@@ -689,7 +703,7 @@ private ObjectNode.ObjectData EncodeEHInfo()
                             if (type.IsCanonicalSubtype(CanonicalFormKind.Any))
                                 ThrowHelper.ThrowInvalidProgramException();
 
-                            var typeSymbol = _compilation.NodeFactory.NecessaryTypeSymbol(type);
+                            var typeSymbol = _compilation.NecessaryTypeSymbolIfPossible(type);
 
                             RelocType rel = (_compilation.NodeFactory.Target.IsWindows) ?
                                 RelocType.IMAGE_REL_BASED_ABSOLUTE :
@@ -1398,7 +1412,7 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO
         private CORINFO_CLASS_STRUCT_* embedClassHandle(CORINFO_CLASS_STRUCT_* handle, ref void* ppIndirection)
         {
             TypeDesc type = HandleToObject(handle);
-            ISymbolNode typeHandleSymbol = _compilation.NodeFactory.NecessaryTypeSymbol(type);
+            ISymbolNode typeHandleSymbol = _compilation.NecessaryTypeSymbolIfPossible(type);
             CORINFO_CLASS_STRUCT_* result = (CORINFO_CLASS_STRUCT_*)ObjectToHandle(typeHandleSymbol);
 
             if (typeHandleSymbol.RepresentsIndirectionCell)
@@ -1578,7 +1592,7 @@ private uint getMethodAttribs(CORINFO_METHOD_STRUCT_* ftn)
         {
             MethodDesc method = HandleToObject(ftn);
             TypeDesc type = method.OwningType;
-            ISymbolNode methodSync = _compilation.NodeFactory.NecessaryTypeSymbol(type);
+            ISymbolNode methodSync = _compilation.NecessaryTypeSymbolIfPossible(type);
 
             void* result = (void*)ObjectToHandle(methodSync);
 

src/tests/nativeaot/SmokeTests/DeadCodeElimination/DeadCodeElimination.cs

@@ -44,14 +44,24 @@ class NeverAllocatedType
             public Type DoSomething() => typeof(UnreferencedType);
         }
 
+#if DEBUG
+        static NeverAllocatedType s_instance = null;
+#else
         static object s_instance = new object[10];
+#endif
 
         public static void Run()
         {
             Console.WriteLine("Testing instance methods on unallocated types");
 
+#if DEBUG
+            if (s_instance != null)
+                s_instance.DoSomething();
+#else
+            // In release builds additionally test that the "is" check didn't introduce the constructed type
             if (s_instance is NeverAllocatedType never)
                 never.DoSomething();
+#endif
 
             ThrowIfPresent(typeof(TestInstanceMethodOptimization), nameof(UnreferencedType));
         }