From 560e60c6ab446f5f26b4ca58fcbba342f1b1073b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 10 Aug 2023 14:33:02 -0700 Subject: [PATCH 01/11] Slow path --- .../nativeaot/BuildIntegration/NativeAOT.natvis | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis index bd5ebc5f660d9d..a4a6d8a5ac256a 100644 --- a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis +++ b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis @@ -61,10 +61,15 @@ - - - - + + + + + + Null {*Get()} From 4755217ef0fe26c8bec90660fa6ffbde1056881e Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 10 Aug 2023 19:11:51 -0700 Subject: [PATCH 02/11] keep an index node for every type --- .../DependencyAnalysis/NodeFactory.cs | 10 +++---- .../Target_ARM64/ARM64ReadyToRunHelperNode.cs | 7 ++++- .../Target_X64/X64ReadyToRunHelperNode.cs | 7 ++++- .../TypeThreadStaticIndexNode.cs | 26 ++++++++----------- 4 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index 23bc01a20065c5..fbb6c08e04006c 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -203,22 +203,20 @@ private void CreateNodeCaches() _threadStatics = new NodeCache(CreateThreadStaticsNode); - TypeThreadStaticIndexNode inlinedThreadStatiscIndexNode = null; if (_inlinedThreadStatics.IsComputed()) { _inlinedThreadStatiscNode = new ThreadStaticsNode(_inlinedThreadStatics, this); - inlinedThreadStatiscIndexNode = new TypeThreadStaticIndexNode(_inlinedThreadStatiscNode); } _typeThreadStaticIndices = new NodeCache(type => { - if (inlinedThreadStatiscIndexNode != null && - _inlinedThreadStatics.GetOffsets().ContainsKey(type)) + if (_inlinedThreadStatics.IsComputed() && + _inlinedThreadStatics.GetOffsets().ContainsKey(type)) { - return inlinedThreadStatiscIndexNode; + return new TypeThreadStaticIndexNode(type, _inlinedThreadStatiscNode); } - return new TypeThreadStaticIndexNode(type); + return new TypeThreadStaticIndexNode(type, null); }); _GCStaticEETypes = new NodeCache((GCPointerMap gcMap) => diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs index 83c2b3db3bab22..e3f2f24df3524e 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs @@ -72,8 +72,13 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, { MetadataType target = (MetadataType)Target; ISortableSymbolNode index = factory.TypeThreadStaticIndex(target); - if (index is TypeThreadStaticIndexNode ti && ti.Type == null) + if (index is TypeThreadStaticIndexNode ti && ti.IsInlined) { + // REVIEW: how to keep a node around? + // we will not use the index node, but need to keep the node around for natvis. + // emit a junk MOV for now + encoder.EmitMOV(encoder.TargetRegister.Result, index); + if (!factory.PreinitializationManager.HasLazyStaticConstructor(target)) { EmitInlineTLSAccess(factory, ref encoder); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs index 8fbd5f609b2546..9ec565393c4784 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs @@ -72,8 +72,13 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo { MetadataType target = (MetadataType)Target; ISortableSymbolNode index = factory.TypeThreadStaticIndex(target); - if (index is TypeThreadStaticIndexNode ti && ti.Type == null) + if (index is TypeThreadStaticIndexNode ti && ti.IsInlined) { + // REVIEW: how to keep a node around? + // we will not use the index node, but need to keep the node around for natvis. + // emit a junk LEA for now + encoder.EmitMOV(encoder.TargetRegister.Result, index); + if (!factory.PreinitializationManager.HasLazyStaticConstructor(target)) { EmitInlineTLSAccess(factory, ref encoder); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs index f91853d1834014..da791c74d7c459 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs @@ -15,19 +15,17 @@ public class TypeThreadStaticIndexNode : DehydratableObjectNode, ISymbolDefiniti private MetadataType _type; private ThreadStaticsNode _inlinedThreadStatics; - public TypeThreadStaticIndexNode(MetadataType type) + public TypeThreadStaticIndexNode(MetadataType type, ThreadStaticsNode inlinedThreadStatics) { _type = type; - } - - public TypeThreadStaticIndexNode(ThreadStaticsNode inlinedThreadStatics) - { _inlinedThreadStatics = inlinedThreadStatics; } + public bool IsInlined => _inlinedThreadStatics != null; + public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) { - sb.Append(_type != null ? nameMangler.NodeMangler.ThreadStaticsIndex(_type) : "_inlinedThreadStaticsIndex"); + sb.Append(nameMangler.NodeMangler.ThreadStaticsIndex(_type)); } public int Offset => 0; @@ -46,9 +44,7 @@ protected override ObjectNodeSection GetDehydratedSection(NodeFactory factory) protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFactory factory) { - ISymbolDefinitionNode node = _type != null ? - factory.TypeThreadStaticsSymbol(_type) : - _inlinedThreadStatics; + ISymbolDefinitionNode node = _inlinedThreadStatics ?? factory.TypeThreadStaticsSymbol(_type); return new DependencyList { @@ -66,12 +62,7 @@ protected override ObjectData GetDehydratableData(NodeFactory factory, bool relo int typeTlsIndex = 0; if (!relocsOnly) { - if (_type != null) - { - ISymbolDefinitionNode node = factory.TypeThreadStaticsSymbol(_type); - typeTlsIndex = ((ThreadStaticsNode)node).IndexFromBeginningOfArray; - } - else + if (IsInlined) { // we use -1 to specify the index of inlined threadstatics, // which are stored separately from uninlined ones. @@ -81,6 +72,11 @@ protected override ObjectData GetDehydratableData(NodeFactory factory, bool relo // is serialized as the item #0 among other storage block types. Debug.Assert(_inlinedThreadStatics.IndexFromBeginningOfArray == 0); } + else + { + ISymbolDefinitionNode node = factory.TypeThreadStaticsSymbol(_type); + typeTlsIndex = ((ThreadStaticsNode)node).IndexFromBeginningOfArray; + } } // needed to construct storage From d79fbf19596f4db75179083e1bd367c6b4e0864d Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 10 Aug 2023 20:07:14 -0700 Subject: [PATCH 03/11] encode the offset --- .../Compiler/DependencyAnalysis/ThreadStaticsNode.cs | 2 ++ .../DependencyAnalysis/TypeThreadStaticIndexNode.cs | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs index 6fa2a7a70cc114..8137cd3082f65f 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs @@ -143,5 +143,7 @@ public override int CompareToImpl(ISortableNode other, CompilerComparer comparer return comparer.Compare(_type, ((ThreadStaticsNode)other)._type); } + + internal int GetTypeStorageOffset(MetadataType type) => _inlined.GetOffsets()[type]; } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs index da791c74d7c459..9a3bbed20e1c71 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs @@ -64,9 +64,11 @@ protected override ObjectData GetDehydratableData(NodeFactory factory, bool relo { if (IsInlined) { - // we use -1 to specify the index of inlined threadstatics, - // which are stored separately from uninlined ones. - typeTlsIndex = -1; + // Inlined threadstatics are stored as a single data block and thus do not need + // an index in the containing storage (since there is none). + // We use a negative index to indicate that. Any negative value would work. + // For the purpose of natvis we will encode the offset of the type storage within the block. + typeTlsIndex = - (_inlinedThreadStatics.GetTypeStorageOffset(_type) + factory.Target.PointerSize); // the type of the storage block for inlined threadstatics, if present, // is serialized as the item #0 among other storage block types. From c6ac3aa114e06842d78a00c6139c4e4f37d68fd9 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 11 Aug 2023 12:17:19 -0700 Subject: [PATCH 04/11] Support inline storage in natvis --- .../nativeaot/BuildIntegration/NativeAOT.natvis | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis index a4a6d8a5ac256a..026646fab26820 100644 --- a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis +++ b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis @@ -64,17 +64,25 @@ - - Null - {*Get()} + + + + + + + + + + Null + {*Get()} - Get() + Get() From d226e9aa23a68ab3129dce71aabb1ef0b6b8d50c Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 11 Aug 2023 13:30:57 -0700 Subject: [PATCH 05/11] store typeManager in TLS root --- src/coreclr/nativeaot/Runtime/thread.cpp | 8 +++++--- src/coreclr/nativeaot/Runtime/thread.h | 7 ++++++- .../src/Internal/Runtime/ThreadStatics.cs | 2 +- .../src/System/Runtime/RuntimeImports.cs | 2 +- .../Compiler/DependencyAnalysis/TlsRootNode.cs | 9 +++++++-- 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp index 9eb428c3d0c402..4c924524fd4d73 100644 --- a/src/coreclr/nativeaot/Runtime/thread.cpp +++ b/src/coreclr/nativeaot/Runtime/thread.cpp @@ -1281,17 +1281,19 @@ InlinedThreadStaticRoot* Thread::GetInlinedThreadStaticList() return m_pInlinedThreadLocalStatics; } -void Thread::RegisterInlinedThreadStaticRoot(InlinedThreadStaticRoot* newRoot) +void Thread::RegisterInlinedThreadStaticRoot(InlinedThreadStaticRoot* newRoot, TypeManager* typeManager) { ASSERT(newRoot->m_next == NULL); newRoot->m_next = m_pInlinedThreadLocalStatics; m_pInlinedThreadLocalStatics = newRoot; + // we do not need typeManager for runtime needs, but it may be useful for debugging purposes. + newRoot->m_typeManager = typeManager; } -COOP_PINVOKE_HELPER(void, RhRegisterInlinedThreadStaticRoot, (Object** root)) +COOP_PINVOKE_HELPER(void, RhRegisterInlinedThreadStaticRoot, (Object** root, TypeManager* typeManager)) { Thread* pCurrentThread = ThreadStore::RawGetCurrentThread(); - pCurrentThread->RegisterInlinedThreadStaticRoot((InlinedThreadStaticRoot*)root); + pCurrentThread->RegisterInlinedThreadStaticRoot((InlinedThreadStaticRoot*)root, typeManager); } // This is function is used to quickly query a value that can uniquely identify a thread diff --git a/src/coreclr/nativeaot/Runtime/thread.h b/src/coreclr/nativeaot/Runtime/thread.h index 1213c3e16ce03b..8d080c7bb4257a 100644 --- a/src/coreclr/nativeaot/Runtime/thread.h +++ b/src/coreclr/nativeaot/Runtime/thread.h @@ -12,6 +12,7 @@ class RuntimeInstance; class ThreadStore; class CLREventStatic; class Thread; +class TypeManager; #ifdef TARGET_UNIX #include "UnixContext.h" @@ -74,8 +75,12 @@ struct GCFrameRegistration struct InlinedThreadStaticRoot { + // The reference to the memory block that stores variables for the current {thread, typeManager} combination Object* m_threadStaticsBase; + // The next root in the list. All roots in the list belong to the same thread, but to a different typeManagers. InlinedThreadStaticRoot* m_next; + // m_typeManager is currently unused, but could be useful when debugging + TypeManager* m_typeManager; }; struct ThreadBuffer @@ -294,7 +299,7 @@ class Thread : private ThreadBuffer Object** GetThreadStaticStorage(); InlinedThreadStaticRoot* GetInlinedThreadStaticList(); - void RegisterInlinedThreadStaticRoot(InlinedThreadStaticRoot* newRoot); + void RegisterInlinedThreadStaticRoot(InlinedThreadStaticRoot* newRoot, TypeManager* typeManager); NATIVE_CONTEXT* GetInterruptedContext(); diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs index 7bfcbff517b22f..61061564af7292 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/ThreadStatics.cs @@ -40,7 +40,7 @@ internal static unsafe object GetInlinedThreadStaticBaseSlow(ref object? threadS object threadStaticBase = AllocateThreadStaticStorageForType(typeManager, 0); // register the storage location with the thread for GC reporting. - RuntimeImports.RhRegisterInlinedThreadStaticRoot(ref threadStorage); + RuntimeImports.RhRegisterInlinedThreadStaticRoot(ref threadStorage, typeManager); // assign the storage block to the storage variable and return threadStorage = threadStaticBase; diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs index 9c652dd69bb9aa..0456e6879d6fde 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -581,7 +581,7 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhRegisterInlinedThreadStaticRoot")] - internal static extern void RhRegisterInlinedThreadStaticRoot(ref object? root); + internal static extern void RhRegisterInlinedThreadStaticRoot(ref object? root, TypeManagerHandle module); [MethodImplAttribute(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhCurrentNativeThreadId")] diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TlsRootNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TlsRootNode.cs index 424ba7130f0df7..a26ff9bec28f6c 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TlsRootNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TlsRootNode.cs @@ -26,10 +26,15 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false) objData.RequireInitialPointerAlignment(); objData.AddSymbol(this); - // root + // storage for InlinedThreadStaticRoot instances + + // m_threadStaticsBase + objData.EmitZeroPointer(); + + // m_next objData.EmitZeroPointer(); - // next + // m_typeManager objData.EmitZeroPointer(); return objData.ToObjectData(); From 8c214a8e12fdd6a4a3295d31ca25762f16a4f2e0 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 11 Aug 2023 17:08:52 -0700 Subject: [PATCH 06/11] test for module --- .../nativeaot/BuildIntegration/NativeAOT.natvis | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis index 026646fab26820..9cad284e0e56dc 100644 --- a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis +++ b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis @@ -70,12 +70,23 @@ GetPerModuleStorage() != nullptr && GetPerModuleStorage()->m_Length > ClassIndex && GetPerTypeStorage() != nullptr "/> - - + + + + + + + + - + From 8ebed9a4aa617b357b159e30ea8916edc78ec3ee Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 12 Aug 2023 19:11:30 -0700 Subject: [PATCH 07/11] root param --- src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis index 9cad284e0e56dc..497ceea990c659 100644 --- a/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis +++ b/src/coreclr/nativeaot/BuildIntegration/NativeAOT.natvis @@ -70,6 +70,8 @@ GetPerModuleStorage() != nullptr && GetPerModuleStorage()->m_Length > ClassIndex && GetPerTypeStorage() != nullptr "/> + + - - + + + - + Null {*Get()} From 7d0928543fc372c0a7385c8721ba303916d60864 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 12 Aug 2023 19:19:40 -0700 Subject: [PATCH 08/11] tweak comments. --- src/coreclr/nativeaot/Runtime/thread.h | 2 +- .../Target_ARM64/ARM64ReadyToRunHelperNode.cs | 2 +- .../DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs | 4 ++-- .../Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/thread.h b/src/coreclr/nativeaot/Runtime/thread.h index 8d080c7bb4257a..f9e5db027be8a8 100644 --- a/src/coreclr/nativeaot/Runtime/thread.h +++ b/src/coreclr/nativeaot/Runtime/thread.h @@ -77,7 +77,7 @@ struct InlinedThreadStaticRoot { // The reference to the memory block that stores variables for the current {thread, typeManager} combination Object* m_threadStaticsBase; - // The next root in the list. All roots in the list belong to the same thread, but to a different typeManagers. + // The next root in the list. All roots in the list belong to the same thread, but to different typeManagers. InlinedThreadStaticRoot* m_next; // m_typeManager is currently unused, but could be useful when debugging TypeManager* m_typeManager; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs index e3f2f24df3524e..2fa917cfd2ef9d 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs @@ -75,7 +75,7 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, if (index is TypeThreadStaticIndexNode ti && ti.IsInlined) { // REVIEW: how to keep a node around? - // we will not use the index node, but need to keep the node around for natvis. + // we do not need the index node to run our code, but need to keep the node around for natvis. // emit a junk MOV for now encoder.EmitMOV(encoder.TargetRegister.Result, index); diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs index 9ec565393c4784..7d0842e122c7dd 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs @@ -75,8 +75,8 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo if (index is TypeThreadStaticIndexNode ti && ti.IsInlined) { // REVIEW: how to keep a node around? - // we will not use the index node, but need to keep the node around for natvis. - // emit a junk LEA for now + // we do not need the index node to run our code, but need to keep the node around for natvis. + // emit a junk MOV for now encoder.EmitMOV(encoder.TargetRegister.Result, index); if (!factory.PreinitializationManager.HasLazyStaticConstructor(target)) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs index 9a3bbed20e1c71..95853fe6cf35cf 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs @@ -65,7 +65,7 @@ protected override ObjectData GetDehydratableData(NodeFactory factory, bool relo if (IsInlined) { // Inlined threadstatics are stored as a single data block and thus do not need - // an index in the containing storage (since there is none). + // an index in the containing storage. // We use a negative index to indicate that. Any negative value would work. // For the purpose of natvis we will encode the offset of the type storage within the block. typeTlsIndex = - (_inlinedThreadStatics.GetTypeStorageOffset(_type) + factory.Target.PointerSize); From 7c293e35e0b5b75ee2e15366455b08f208533fa8 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 12 Aug 2023 21:01:42 -0700 Subject: [PATCH 09/11] make junk MOV`s unreachable dead code --- .../Target_ARM64/ARM64ReadyToRunHelperNode.cs | 10 +++++----- .../Target_X64/X64ReadyToRunHelperNode.cs | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs index 2fa917cfd2ef9d..1be3ef69e9cf6b 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs @@ -74,11 +74,6 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, ISortableSymbolNode index = factory.TypeThreadStaticIndex(target); if (index is TypeThreadStaticIndexNode ti && ti.IsInlined) { - // REVIEW: how to keep a node around? - // we do not need the index node to run our code, but need to keep the node around for natvis. - // emit a junk MOV for now - encoder.EmitMOV(encoder.TargetRegister.Result, index); - if (!factory.PreinitializationManager.HasLazyStaticConstructor(target)) { EmitInlineTLSAccess(factory, ref encoder); @@ -99,6 +94,11 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, encoder.EmitJNE(factory.HelperEntrypoint(HelperEntrypoint.EnsureClassConstructorRunAndReturnThreadStaticBase)); EmitInlineTLSAccess(factory, ref encoder); } + + // REVIEW: how to keep a node around? + // we do not need the index node to run our code, but need to keep the node around for natvis. + // emit a junk MOV for now (this code is unreachable) + encoder.EmitMOV(encoder.TargetRegister.Result, index); } else { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs index 7d0842e122c7dd..a48b918abef44d 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs @@ -74,11 +74,6 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo ISortableSymbolNode index = factory.TypeThreadStaticIndex(target); if (index is TypeThreadStaticIndexNode ti && ti.IsInlined) { - // REVIEW: how to keep a node around? - // we do not need the index node to run our code, but need to keep the node around for natvis. - // emit a junk MOV for now - encoder.EmitMOV(encoder.TargetRegister.Result, index); - if (!factory.PreinitializationManager.HasLazyStaticConstructor(target)) { EmitInlineTLSAccess(factory, ref encoder); @@ -99,6 +94,11 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo EmitInlineTLSAccess(factory, ref encoder); } + + // REVIEW: how to keep a node around? + // we do not need the index node to run our code, but need to keep the node around for natvis. + // emit a junk MOV for now (this code is unreachable) + encoder.EmitMOV(encoder.TargetRegister.Result, index); } else { From 5535f1e978b6473e1ea2529d006a08dc7d65549d Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 14 Aug 2023 10:00:48 -0700 Subject: [PATCH 10/11] Keep index nodes for inlined threadstatics unconditionally, remove "deadcode MOV" hack --- .../Target_ARM64/ARM64ReadyToRunHelperNode.cs | 5 ----- .../DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs | 5 ----- .../Compiler/DependencyAnalysis/ThreadStaticsNode.cs | 4 +++- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs index 1be3ef69e9cf6b..0fca02c971a4f1 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_ARM64/ARM64ReadyToRunHelperNode.cs @@ -94,11 +94,6 @@ protected override void EmitCode(NodeFactory factory, ref ARM64Emitter encoder, encoder.EmitJNE(factory.HelperEntrypoint(HelperEntrypoint.EnsureClassConstructorRunAndReturnThreadStaticBase)); EmitInlineTLSAccess(factory, ref encoder); } - - // REVIEW: how to keep a node around? - // we do not need the index node to run our code, but need to keep the node around for natvis. - // emit a junk MOV for now (this code is unreachable) - encoder.EmitMOV(encoder.TargetRegister.Result, index); } else { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs index a48b918abef44d..4d6b4b01ba180c 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_X64/X64ReadyToRunHelperNode.cs @@ -94,11 +94,6 @@ protected override void EmitCode(NodeFactory factory, ref X64Emitter encoder, bo EmitInlineTLSAccess(factory, ref encoder); } - - // REVIEW: how to keep a node around? - // we do not need the index node to run our code, but need to keep the node around for natvis. - // emit a junk MOV for now (this code is unreachable) - encoder.EmitMOV(encoder.TargetRegister.Result, index); } else { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs index 8137cd3082f65f..8935ce8d3d0561 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ThreadStaticsNode.cs @@ -73,7 +73,6 @@ public override IEnumerable GetStaticDependencies(NodeFacto if (_type != null) { - if (factory.PreinitializationManager.HasEagerStaticConstructor(_type)) { result.Add(new DependencyListEntry(factory.EagerCctorIndirection(_type.GetStaticConstructor()), "Eager .cctor")); @@ -90,6 +89,9 @@ public override IEnumerable GetStaticDependencies(NodeFacto result.Add(new DependencyListEntry(factory.EagerCctorIndirection(type.GetStaticConstructor()), "Eager .cctor")); } + // inlined threadstatics do not need the index for execution, but may need it for debug visualization. + result.Add(new DependencyListEntry(factory.TypeThreadStaticIndex(type), "ThreadStatic index for debug visualization")); + ModuleUseBasedDependencyAlgorithm.AddDependenciesDueToModuleUse(ref result, factory, type.Module); } } From dbecd410429d8ae774c5cd357d692be2044e19de Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Mon, 14 Aug 2023 15:13:35 -0700 Subject: [PATCH 11/11] Update src/coreclr/nativeaot/Runtime/thread.h --- src/coreclr/nativeaot/Runtime/thread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/Runtime/thread.h b/src/coreclr/nativeaot/Runtime/thread.h index f9e5db027be8a8..7aa473eb6763d4 100644 --- a/src/coreclr/nativeaot/Runtime/thread.h +++ b/src/coreclr/nativeaot/Runtime/thread.h @@ -79,7 +79,7 @@ struct InlinedThreadStaticRoot Object* m_threadStaticsBase; // The next root in the list. All roots in the list belong to the same thread, but to different typeManagers. InlinedThreadStaticRoot* m_next; - // m_typeManager is currently unused, but could be useful when debugging + // m_typeManager is used by NativeAOT.natvis when debugging TypeManager* m_typeManager; };