From 8e21763bbac7e22ab20b1faffe4dc164ba26ae51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 6 Dec 2017 22:02:05 +0100 Subject: [PATCH 1/3] Fix todo in initClass --- src/JitInterface/src/CorInfoImpl.cs | 47 ++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs index 9c602cc3a31..184c42c3ab6 100644 --- a/src/JitInterface/src/CorInfoImpl.cs +++ b/src/JitInterface/src/CorInfoImpl.cs @@ -1726,10 +1726,10 @@ private CorInfoInitClassResult initClass(CORINFO_FIELD_STRUCT_* field, CORINFO_M return CorInfoInitClassResult.CORINFO_INITCLASS_NOT_REQUIRED; } + MetadataType typeToInit = (MetadataType)type; + if (fd == null) { - MetadataType typeToInit = (MetadataType)type; - if (typeToInit.IsBeforeFieldInit) { // We can wait for field accesses to run .cctor @@ -1765,8 +1765,47 @@ private CorInfoInitClassResult initClass(CORINFO_FIELD_STRUCT_* field, CORINFO_M return CorInfoInitClassResult.CORINFO_INITCLASS_USE_HELPER | CorInfoInitClassResult.CORINFO_INITCLASS_DONT_INLINE; } - // TODO: before giving up and asking to generate a helper call, check to see if this is some pattern we can - // prove doesn't need initclass anymore because we initialized it earlier. + // + // Try to prove that the initialization is not necessary because of nesting + // + + if (fd == null) + { + // Handled above + Debug.Assert(!typeToInit.IsBeforeFieldInit); + + // Note that jit has both methods the same if asking whether to emit cctor + // for a given method's code (as opposed to inlining codegen). + MethodDesc contextMethod = methodFromContext(context); + if (contextMethod != MethodBeingCompiled && type == MethodBeingCompiled.OwningType) + { + // If we're inling a call to a method in our own type, then we should already + // have triggered the .cctor when caller was itself called. + return CorInfoInitClassResult.CORINFO_INITCLASS_NOT_REQUIRED; + } + } + else + { + // This optimization may cause static fields in reference types to be accessed without cctor being triggered + // for NULL "this" object. It does not conform with what the spec says. However, we have been historically + // doing it for perf reasons. + if (!typeToInit.IsValueType && !typeToInit.IsBeforeFieldInit) + { + if (typeToInit == typeFromContext(context) || typeToInit == MethodBeingCompiled.OwningType) + { + // The class will be initialized by the time we access the field. + return CorInfoInitClassResult.CORINFO_INITCLASS_NOT_REQUIRED; + } + } + + // If we are currently compiling the class constructor for this static field access then we can skip the initClass + if (MethodBeingCompiled.OwningType == typeToInit && MethodBeingCompiled.IsStaticConstructor) + { + // The class will be initialized by the time we access the field. + return CorInfoInitClassResult.CORINFO_INITCLASS_NOT_REQUIRED; + } + + } return CorInfoInitClassResult.CORINFO_INITCLASS_USE_HELPER; } From 256c85a366b333d09bed2f3b854c3cda425f4adf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 6 Dec 2017 22:03:14 +0100 Subject: [PATCH 2/3] WIP --- .../SymbolWithOffsetNode.cs | 60 +++++++++++++++++++ .../src/ILCompiler.Compiler.csproj | 1 + src/JitInterface/src/CorInfoImpl.cs | 55 +++++++++++------ 3 files changed, 97 insertions(+), 19 deletions(-) create mode 100644 src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/SymbolWithOffsetNode.cs diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/SymbolWithOffsetNode.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/SymbolWithOffsetNode.cs new file mode 100644 index 00000000000..067806dde1d --- /dev/null +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/SymbolWithOffsetNode.cs @@ -0,0 +1,60 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. +using System; +using System.Collections.Generic; + +using ILCompiler.DependencyAnalysisFramework; + +using Internal.Text; + +namespace ILCompiler.DependencyAnalysis +{ + internal class SymbolWithOffsetNode : DependencyNodeCore, ISymbolNode + { + private ISymbolNode _target; + private int _offset; + + public SymbolWithOffsetNode(ISymbolNode target, int offset) + { + _target = target; + _offset = offset; + } + + public int Offset => _offset; + + public override bool InterestingForDynamicDependencyAnalysis => false; + public override bool HasDynamicDependencies => false; + public override bool HasConditionalStaticDependencies => false; + public override bool StaticDependenciesAreComputed => true; + public bool RepresentsIndirectionCell => false; + + public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) + { + _target.AppendMangledName(nameMangler, sb); + } + + public override IEnumerable GetConditionalStaticDependencies(NodeFactory context) + { + return null; + } + + public override IEnumerable GetStaticDependencies(NodeFactory context) + { + return new DependencyListEntry[] + { + new DependencyListEntry(_target, "Target"), + }; + } + + public override IEnumerable SearchDynamicDependencies(List> markedNodes, int firstNode, NodeFactory context) + { + return null; + } + + protected override string GetName(NodeFactory context) + { + return "__offs_" + _offset.ToStringInvariant() + "_from_" + _target.GetMangledName(context.NameMangler); + } + } +} diff --git a/src/ILCompiler.Compiler/src/ILCompiler.Compiler.csproj b/src/ILCompiler.Compiler/src/ILCompiler.Compiler.csproj index b56b4ec1557..5a24fb7c781 100644 --- a/src/ILCompiler.Compiler/src/ILCompiler.Compiler.csproj +++ b/src/ILCompiler.Compiler/src/ILCompiler.Compiler.csproj @@ -114,6 +114,7 @@ + diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs index 184c42c3ab6..e91f88c23e1 100644 --- a/src/JitInterface/src/CorInfoImpl.cs +++ b/src/JitInterface/src/CorInfoImpl.cs @@ -2105,9 +2105,8 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET pResult.fieldLookup = CreateConstLookupToSymbol(helper); } } - else + else if (field.IsThreadStatic || field.HasGCStaticBase) { - fieldAccessor = CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER; pResult.helper = CorInfoHelpFunc.CORINFO_HELP_READYTORUN_STATIC_BASE; @@ -2116,23 +2115,10 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET { helperId = ReadyToRunHelperId.GetThreadStaticBase; } - else if (field.HasGCStaticBase) - { - helperId = ReadyToRunHelperId.GetGCStaticBase; - } else { - var owningType = field.OwningType; - if ((owningType.IsWellKnownType(WellKnownType.IntPtr) || - owningType.IsWellKnownType(WellKnownType.UIntPtr)) && - field.Name == "Zero") - { - fieldAccessor = CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_INTRINSIC_ZERO; - } - else - { - helperId = ReadyToRunHelperId.GetNonGCStaticBase; - } + Debug.Assert(field.HasGCStaticBase); + helperId = ReadyToRunHelperId.GetGCStaticBase; } if (helperId != ReadyToRunHelperId.Invalid) @@ -2140,6 +2126,26 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET pResult.fieldLookup = CreateConstLookupToSymbol(_compilation.NodeFactory.ReadyToRunHelper(helperId, field.OwningType)); } } + else + { + var owningType = field.OwningType; + if ((owningType.IsWellKnownType(WellKnownType.IntPtr) || + owningType.IsWellKnownType(WellKnownType.UIntPtr)) && + field.Name == "Zero") + { + fieldAccessor = CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_INTRINSIC_ZERO; + } + else + { + fieldAccessor = CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_STATIC_ADDRESS; + + // We are not going through a helper. The constructor has to be triggered explicitly. + if (_compilation.HasLazyStaticConstructor(field.OwningType)) + { + //fieldFlags |= CORINFO_FIELD_FLAGS.CORINFO_FLG_FIELD_INITCLASS; + } + } + } } else { @@ -3357,8 +3363,19 @@ private uint getClassDomainID(CORINFO_CLASS_STRUCT_* cls, ref void* ppIndirectio private void* getFieldAddress(CORINFO_FIELD_STRUCT_* field, ref void* ppIndirection) { FieldDesc fieldDesc = HandleToObject(field); - Debug.Assert(fieldDesc.HasRva); - return (void*)ObjectToHandle(_compilation.GetFieldRvaData(fieldDesc)); + if (fieldDesc.HasRva) + { + return (void*)ObjectToHandle(_compilation.GetFieldRvaData(fieldDesc)); + } + else + { + var owningType = fieldDesc.OwningType; + Debug.Assert(!fieldDesc.IsThreadStatic && !fieldDesc.HasGCStaticBase); + + ISymbolNode baseAddr = _compilation.NodeFactory.TypeNonGCStaticsSymbol((MetadataType)owningType); + + return (void*)ObjectToHandle(new SymbolWithOffsetNode(baseAddr, fieldDesc.Offset.AsInt)); + } } private IntPtr getVarArgsHandle(CORINFO_SIG_INFO* pSig, ref void* ppIndirection) From fa2ff1524bae093040ac35cd29a409b3536868d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 18 Dec 2017 16:05:47 +0100 Subject: [PATCH 3/3] Finish --- .../src/Compiler/Compilation.cs | 14 ++++++++ .../DependencyAnalysis/NodeFactory.cs | 35 ++++++++++++++++++- src/JitInterface/src/CorInfoImpl.cs | 16 ++------- .../Runtime/JitSupport/JitCompilation.cs | 5 +++ 4 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/ILCompiler.Compiler/src/Compiler/Compilation.cs b/src/ILCompiler.Compiler/src/Compiler/Compilation.cs index 0c2ba245333..eae591237ab 100644 --- a/src/ILCompiler.Compiler/src/Compiler/Compilation.cs +++ b/src/ILCompiler.Compiler/src/Compiler/Compilation.cs @@ -113,6 +113,20 @@ public ObjectNode GetFieldRvaData(FieldDesc field) } } + public ISymbolNode GetFieldData(FieldDesc field) + { + if (field.HasRva) + { + return (ISymbolNode)GetFieldRvaData(field); + } + else + { + Debug.Assert(field.IsStatic && !field.IsThreadStatic && !field.HasGCStaticBase); + ISymbolNode baseAddr = NodeFactory.TypeNonGCStaticsSymbol((MetadataType)field.OwningType); + return NodeFactory.SymbolWithOffset(baseAddr, field.Offset.AsInt); + } + } + public bool HasLazyStaticConstructor(TypeDesc type) { return TypeSystemContext.HasLazyStaticConstructor(type); diff --git a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs index 2a25d967bce..9033cff6bb7 100644 --- a/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs @@ -414,6 +414,11 @@ private void CreateNodeCaches() return new NamedJumpStubNode(id.Item1, id.Item2); }); + _symbolWithOffsetNodes = new NodeCache((SymbolWithOffsetKey key) => + { + return new SymbolWithOffsetNode(key.Symbol, key.Offset); + }); + _vTableNodes = new NodeCache((TypeDesc type ) => { if (CompilationModuleGroup.ShouldProduceFullVTable(type)) @@ -973,7 +978,14 @@ public ISymbolNode NamedJumpStub(string name, ISymbolNode target) { return _namedJumpStubNodes.GetOrAdd(new Tuple(name, target)); } - + + private NodeCache _symbolWithOffsetNodes; + + public ISymbolNode SymbolWithOffset(ISymbolNode symbol, int offset) + { + return _symbolWithOffsetNodes.GetOrAdd(new SymbolWithOffsetKey(symbol, offset)); + } + /// /// Returns alternative symbol name that object writer should produce for given symbols /// in addition to the regular one. @@ -1142,6 +1154,27 @@ public override int GetHashCode() } } + protected struct SymbolWithOffsetKey : IEquatable + { + public readonly ISymbolNode Symbol; + public readonly int Offset; + + public SymbolWithOffsetKey(ISymbolNode symbol, int offset) + { + Symbol = symbol; + Offset = offset; + } + + public bool Equals(SymbolWithOffsetKey other) => Symbol == other.Symbol && Offset == other.Offset; + public override bool Equals(object obj) => obj is SymbolWithOffsetKey && Equals((SymbolWithOffsetKey)obj); + public override int GetHashCode() + { + int hashCode = Symbol.GetHashCode(); + hashCode = hashCode * 23 + Offset; + return hashCode; + } + } + protected struct ReadOnlyDataBlobKey : IEquatable { public readonly Utf8String Name; diff --git a/src/JitInterface/src/CorInfoImpl.cs b/src/JitInterface/src/CorInfoImpl.cs index ac57ff486cc..17352644f44 100644 --- a/src/JitInterface/src/CorInfoImpl.cs +++ b/src/JitInterface/src/CorInfoImpl.cs @@ -2172,7 +2172,7 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET // We are not going through a helper. The constructor has to be triggered explicitly. if (_compilation.HasLazyStaticConstructor(field.OwningType)) { - //fieldFlags |= CORINFO_FIELD_FLAGS.CORINFO_FLG_FIELD_INITCLASS; + fieldFlags |= CORINFO_FIELD_FLAGS.CORINFO_FLG_FIELD_INITCLASS; } } } @@ -3393,19 +3393,7 @@ private uint getClassDomainID(CORINFO_CLASS_STRUCT_* cls, ref void* ppIndirectio private void* getFieldAddress(CORINFO_FIELD_STRUCT_* field, ref void* ppIndirection) { FieldDesc fieldDesc = HandleToObject(field); - if (fieldDesc.HasRva) - { - return (void*)ObjectToHandle(_compilation.GetFieldRvaData(fieldDesc)); - } - else - { - var owningType = fieldDesc.OwningType; - Debug.Assert(!fieldDesc.IsThreadStatic && !fieldDesc.HasGCStaticBase); - - ISymbolNode baseAddr = _compilation.NodeFactory.TypeNonGCStaticsSymbol((MetadataType)owningType); - - return (void*)ObjectToHandle(new SymbolWithOffsetNode(baseAddr, fieldDesc.Offset.AsInt)); - } + return (void*)ObjectToHandle(_compilation.GetFieldData(fieldDesc)); } private IntPtr getVarArgsHandle(CORINFO_SIG_INFO* pSig, ref void* ppIndirection) diff --git a/src/System.Private.Jit/src/Internal/Runtime/JitSupport/JitCompilation.cs b/src/System.Private.Jit/src/Internal/Runtime/JitSupport/JitCompilation.cs index e628978b9ee..f4f6919f0aa 100644 --- a/src/System.Private.Jit/src/Internal/Runtime/JitSupport/JitCompilation.cs +++ b/src/System.Private.Jit/src/Internal/Runtime/JitSupport/JitCompilation.cs @@ -47,6 +47,11 @@ public ObjectNode GetFieldRvaData(FieldDesc field) throw new NotImplementedException(); } + public ObjectNode GetFieldData(FieldDesc field) + { + throw new NotImplementedException(); + } + internal MethodIL GetMethodIL(MethodDesc method) { // Flush the cache when it grows too big