PR feedback

dotnet · May 4, 2023 · 419af38 · 419af38
1 parent 1ee3b96
commit 419af38
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 59 deletions.
diff --git a/src/coreclr/nativeaot/Runtime/threadstore.cpp b/src/coreclr/nativeaot/Runtime/threadstore.cpp
@@ -432,9 +432,8 @@ C_ASSERT(sizeof(Thread) == sizeof(ThreadBuffer));
 __thread ThreadBuffer tls_CurrentThread;
 
 // the root of inlined threadstatics storage
-// there is only one now.
-// eventually this will be emitted by ILC and we may have as many such variables
-// as types that have threadstatics
+// there is only one now,
+// eventually this will be emitted by ILC and we may have more than one such variable
 __thread InlinedThreadStaticRoot tls_InlinedThreadStatics;
 #endif
 
@@ -516,4 +515,4 @@ void ThreadStore::SaveCurrentThreadOffsetForDAC()
 {
 }
 
-#endif // _WIN32
+#endif // _WIN32
diff --git a/src/coreclr/nativeaot/Runtime/threadstore.inl b/src/coreclr/nativeaot/Runtime/threadstore.inl
@@ -6,9 +6,8 @@
 EXTERN_C __declspec(selectany) __declspec(thread) ThreadBuffer tls_CurrentThread;
 
 // the root of inlined threadstatics storage
-// there is only one now.
-// eventually this will be emitted by ILC and we may have as many such variables
-// as types that have threadstatics
+// there is only one now,
+// eventually this will be emitted by ILC and we may have more than one such variable
 EXTERN_C __declspec(selectany) __declspec(thread) InlinedThreadStaticRoot tls_InlinedThreadStatics;
 #else
 EXTERN_C __thread ThreadBuffer tls_CurrentThread;

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs
@@ -97,59 +97,23 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
             return result;
         }
 
-        public override bool HasConditionalStaticDependencies
-        {
-            get
-            {
-                if (_type != null)
-                {
-                    return _type.ConvertToCanonForm(CanonicalFormKind.Specific) != _type;
-                }
-                else
-                {
-                    foreach (var type in _inlined.GetTypes())
-                    {
-                        if (type.ConvertToCanonForm(CanonicalFormKind.Specific) != type)
-                        {
-                            return true;
-                        }
-                    }
-
-                    return false;
-                }
-            }
-        }
-
-        private IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependenciesMany(NodeFactory factory)
-        {
-            foreach (var type in _inlined.GetTypes())
-            {
-                if (type.ConvertToCanonForm(CanonicalFormKind.Specific) != type)
-                {
-                    yield return new CombinedDependencyListEntry(factory.NecessaryTypeSymbol(type),
-                        factory.NativeLayout.TemplateTypeLayout(type.ConvertToCanonForm(CanonicalFormKind.Specific)),
-                        "Keeping track of template-constructable type static bases");
-                }
-            }
-        }
+        public override bool HasConditionalStaticDependencies =>
+            _type != null ?
+                _type.ConvertToCanonForm(CanonicalFormKind.Specific) != _type:
+                false;
 
         public override IEnumerable<CombinedDependencyListEntry> GetConditionalStaticDependencies(NodeFactory factory)
         {
-            if (_type != null)
-            {
-                // If we have a type loader template for this type, we need to keep track of the generated
-                // bases in the type info hashtable. The type symbol node does such accounting.
-                return new CombinedDependencyListEntry[]
-                {
-                    new CombinedDependencyListEntry(factory.NecessaryTypeSymbol(_type),
-                        factory.NativeLayout.TemplateTypeLayout(_type.ConvertToCanonForm(CanonicalFormKind.Specific)),
-                        "Keeping track of template-constructable type static bases"),
-                };
-            }
-            else
+            Debug.Assert(_type != null);
+
+            // If we have a type loader template for this type, we need to keep track of the generated
+            // bases in the type info hashtable. The type symbol node does such accounting.
+            return new CombinedDependencyListEntry[]
             {
-                return GetConditionalStaticDependenciesMany(factory);
-            }
+                new CombinedDependencyListEntry(factory.NecessaryTypeSymbol(_type),
+                    factory.NativeLayout.TemplateTypeLayout(_type.ConvertToCanonForm(CanonicalFormKind.Specific)),
+                    "Keeping track of template-constructable type static bases"),
+            };
         }
 
         public override bool StaticDependenciesAreComputed => true;

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs
@@ -714,9 +714,9 @@ public ScannedInlinedThreadStatics(NodeFactory factory, ImmutableArray<Dependenc
                     {
                         ThreadStaticsNode threadStaticNode = threadStaticNodes[i];
                         MetadataType t = threadStaticNode.Type;
-                        // REVIEW: how to filter these more precisely?
-                        //       List<int> is ok, but List<object> is not.
-                        if (t.HasInstantiation)
+
+                        // do not inline storage for shared generics
+                        if (t.ConvertToCanonForm(CanonicalFormKind.Specific) != t)
                             continue;
 
 #if DEBUG