Address code review feedback.

erozenfeld · erozenfeld · commit b51ff00553b2 · 2018-11-06T19:31:43.000-08:00
diff --git a/src/jit/compmemkind.h b/src/jit/compmemkind.h
@@ -53,6 +53,7 @@ CompMemKindMacro(Unknown)
 CompMemKindMacro(RangeCheck)
 CompMemKindMacro(CopyProp)
 CompMemKindMacro(SideEffects)
+CompMemKindMacro(ObjectAllocator)
 //clang-format on
 
 #undef CompMemKindMacro
diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
@@ -16734,10 +16734,14 @@ void Compiler::fgMorph()
     // local variable allocation on the stack.
     ObjectAllocator objectAllocator(this); // PHASE_ALLOCATE_OBJECTS
 
+// TODO-ObjectStackAllocation: Enable the optimization for architectures using
+// JIT32_GCENCODER (i.e., x86).
+#ifndef JIT32_GCENCODER
     if (JitConfig.JitObjectStackAllocation() && !opts.MinOpts() && !opts.compDbgCode)
     {
         objectAllocator.EnableObjectStackAllocation();
     }
+#endif // JIT32_GCENCODER
 
     objectAllocator.Run();
 
diff --git a/src/jit/objectalloc.cpp b/src/jit/objectalloc.cpp
@@ -53,15 +53,6 @@ void ObjectAllocator::DoPhase()
     }
 }
 
-struct ObjectAllocator::BuildConnGraphVisitorCallbackData
-{
-    BuildConnGraphVisitorCallbackData(ObjectAllocator* objectAllocator) : m_objectAllocator(objectAllocator)
-    {
-    }
-
-    ObjectAllocator* m_objectAllocator;
-};
-
 //------------------------------------------------------------------------------
 // MarkLclVarAsEscaping : Mark local variable as escaping.
 //
@@ -114,7 +105,7 @@ void ObjectAllocator::DoAnalysis()
     if (comp->lvaCount > 0)
     {
         m_EscapingPointers         = BitVecOps::MakeEmpty(&m_bitVecTraits);
-        m_ConnGraphAdjacencyMatrix = new (comp->getAllocator()) BitSetShortLongRep[comp->lvaCount];
+        m_ConnGraphAdjacencyMatrix = new (comp->getAllocator(CMK_ObjectAllocator)) BitSetShortLongRep[comp->lvaCount];
 
         MarkEscapingVarsAndBuildConnGraph();
         ComputeEscapingNodes(&m_bitVecTraits, m_EscapingPointers);
@@ -141,7 +132,47 @@ void ObjectAllocator::DoAnalysis()
 
 void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph()
 {
-    BuildConnGraphVisitorCallbackData callbackData(this);
+    class BuildConnGraphVisitor final : public GenTreeVisitor<BuildConnGraphVisitor>
+    {
+        ObjectAllocator* m_allocator;
+
+    public:
+        enum
+        {
+            DoPreOrder    = true,
+            DoLclVarsOnly = true,
+            ComputeStack  = true,
+        };
+
+        BuildConnGraphVisitor(ObjectAllocator* allocator)
+            : GenTreeVisitor<BuildConnGraphVisitor>(allocator->comp), m_allocator(allocator)
+        {
+        }
+
+        Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
+        {
+            GenTree* tree = *use;
+            assert(tree != nullptr);
+            assert(tree->IsLocal());
+
+            var_types type = tree->TypeGet();
+            if ((tree->OperGet() == GT_LCL_VAR) && (type == TYP_REF || type == TYP_BYREF || type == TYP_I_IMPL))
+            {
+                unsigned int lclNum = tree->AsLclVar()->GetLclNum();
+                assert(tree == m_ancestors.Index(0));
+
+                if (m_allocator->CanLclVarEscapeViaParentStack(&m_ancestors, lclNum))
+                {
+                    if (!m_allocator->IsLclVarEscaping(lclNum))
+                    {
+                        JITDUMP("V%02u first escapes via [%06u]\n", lclNum, m_compiler->dspTreeID(tree));
+                    }
+                    m_allocator->MarkLclVarAsEscaping(lclNum);
+                }
+            }
+            return Compiler::fgWalkResult::WALK_CONTINUE;
+        }
+    };
 
     for (unsigned int lclNum = 0; lclNum < comp->lvaCount; ++lclNum)
     {
@@ -170,10 +201,8 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph()
     {
         for (GenTreeStmt* stmt = block->firstStmt(); stmt; stmt = stmt->gtNextStmt)
         {
-            const bool lclVarsOnly  = true;
-            const bool computeStack = true;
-
-            comp->fgWalkTreePre(&stmt->gtStmtExpr, BuildConnGraphVisitor, &callbackData, lclVarsOnly, computeStack);
+            BuildConnGraphVisitor buildConnGraphVisitor(this);
+            buildConnGraphVisitor.WalkTree(&stmt->gtStmtExpr, nullptr);
         }
     }
 }
@@ -423,55 +452,13 @@ unsigned int ObjectAllocator::MorphAllocObjNodeIntoStackAlloc(GenTreeAllocObj* a
     return lclNum;
 }
 
-//------------------------------------------------------------------------
-// BuildConnGraphVisitor: Check if the current tree is a local variable that can point to an object
-//                        and is escaping. Update the connection graph as necessary.
-//
-// Arguments:
-//    pTree - Tree being visited
-//    data  - Walker data
-//
-// Return Value:
-//    Always returns fgWalkResult::WALK_CONTINUE
-
-Compiler::fgWalkResult ObjectAllocator::BuildConnGraphVisitor(GenTree** pTree, Compiler::fgWalkData* data)
-{
-    GenTree* tree = *pTree;
-    assert(tree);
-
-    var_types type = tree->TypeGet();
-    if (tree->OperGet() == GT_LCL_VAR && (type == TYP_REF || type == TYP_BYREF || type == TYP_I_IMPL))
-    {
-        Compiler*                          compiler = data->compiler;
-        GenTree*                           parent   = data->parent;
-        BuildConnGraphVisitorCallbackData* callbackData =
-            reinterpret_cast<BuildConnGraphVisitorCallbackData*>(data->pCallbackData);
-        ObjectAllocator* objectAllocator = callbackData->m_objectAllocator;
-
-        unsigned int lclNum = tree->AsLclVar()->GetLclNum();
-        assert(tree == data->parentStack->Index(0));
-        assert(data->parent == data->parentStack->Index(1));
-
-        if (objectAllocator->CanLclVarEscapeViaParentStack(data->parentStack, lclNum, data->compiler))
-        {
-            if (!objectAllocator->IsLclVarEscaping(lclNum))
-            {
-                JITDUMP("V%02u first escapes (2) via [%06u]\n", lclNum, tree->gtTreeID);
-            }
-            objectAllocator->MarkLclVarAsEscaping(lclNum);
-        }
-    }
-    return Compiler::fgWalkResult::WALK_CONTINUE;
-}
-
 //------------------------------------------------------------------------
 // CanLclVarEscapeViaParentStack: Check if the local variable escapes via the given parent stack.
 //                                Update the connection graph as necessary.
 //
 // Arguments:
 //    parentStack     - Parent stack of the current visit
 //    lclNum          - Local variable number
-//    compiler        - Compiler instance
 //
 // Return Value:
 //    true if the local can escape via the parent stack; false otherwise
@@ -480,17 +467,15 @@ Compiler::fgWalkResult ObjectAllocator::BuildConnGraphVisitor(GenTree** pTree, C
 //    The method currently treats all locals assigned to a field as escaping.
 //    The can potentially be tracked by special field edges in the connection graph.
 
-bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parentStack,
-                                                    unsigned int          lclNum,
-                                                    Compiler*             compiler)
+bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parentStack, unsigned int lclNum)
 {
     assert(parentStack != nullptr);
     int parentIndex = 1;
 
-    bool done                          = false;
+    bool keepChecking                  = true;
     bool canLclVarEscapeViaParentStack = true;
 
-    while (!done)
+    while (keepChecking)
     {
         if (parentStack->Height() <= parentIndex)
         {
@@ -501,7 +486,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parent
         canLclVarEscapeViaParentStack = true;
         GenTree* tree                 = parentStack->Index(parentIndex - 1);
         GenTree* parent               = parentStack->Index(parentIndex);
-        done                          = true;
+        keepChecking                  = false;
 
         switch (parent->OperGet())
         {
@@ -566,7 +551,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parent
             case GT_ADD:
                 // Check whether the local escapes via its grandparent.
                 ++parentIndex;
-                done = false;
+                keepChecking = true;
                 break;
 
             case GT_FIELD:
@@ -578,7 +563,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parent
                 {
                     // Check if the address of the field/ind escapes.
                     parentIndex += 2;
-                    done = false;
+                    keepChecking = true;
                 }
                 else
                 {
@@ -594,7 +579,7 @@ bool ObjectAllocator::CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parent
 
                 if (asCall->gtCallType == CT_HELPER)
                 {
-                    const CorInfoHelpFunc helperNum = compiler->eeGetHelperNum(asCall->gtCallMethHnd);
+                    const CorInfoHelpFunc helperNum = comp->eeGetHelperNum(asCall->gtCallMethHnd);
 
                     if (Compiler::s_helperCallProperties.IsPure(helperNum))
                     {
@@ -644,46 +629,50 @@ Compiler::fgWalkResult ObjectAllocator::AssertWhenAllocObjFoundVisitor(GenTree**
 
 void ObjectAllocator::RewriteUses()
 {
-    BasicBlock* block;
-
-    foreach_block(comp, block)
+    class RewriteUsesVisitor final : public GenTreeVisitor<RewriteUsesVisitor>
     {
-        for (GenTreeStmt* stmt = block->firstStmt(); stmt; stmt = stmt->gtNextStmt)
+        ObjectAllocator* m_allocator;
+
+    public:
+        enum
         {
-            const bool lclVarsOnly  = true;
-            const bool computeStack = false;
+            DoPreOrder    = true,
+            DoLclVarsOnly = true,
+        };
 
-            comp->fgWalkTreePre(&stmt->gtStmtExpr, RewriteUsesVisitor, this, lclVarsOnly, computeStack);
+        RewriteUsesVisitor(ObjectAllocator* allocator)
+            : GenTreeVisitor<RewriteUsesVisitor>(allocator->comp), m_allocator(allocator)
+        {
         }
-    }
-}
 
-//------------------------------------------------------------------------
-// RewriteUsesVisitor: Replace a use of the newobj temp with address of the stack local.
-//
-// Arguments:
-//    pTree   - Tree to examine
-//    data    - Walker data
-//
-// Return Value:
-//    Always returns fgWalkResult::WALK_CONTINUE
+        Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
+        {
+            GenTree* tree = *use;
+            assert(tree != nullptr);
+            assert(tree->IsLocal());
 
-Compiler::fgWalkResult ObjectAllocator::RewriteUsesVisitor(GenTree** pTree, Compiler::fgWalkData* data)
-{
-    ObjectAllocator* allocator = reinterpret_cast<ObjectAllocator*>(data->pCallbackData);
-    GenTree*         tree      = *pTree;
-    assert(tree != nullptr);
-    assert(tree->IsLocal());
+            const unsigned int lclNum    = tree->AsLclVarCommon()->gtLclNum;
+            unsigned int       newLclNum = BAD_VAR_NUM;
+
+            if (m_allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum))
+            {
+                GenTree* newTree =
+                    m_compiler->gtNewOperNode(GT_ADDR, TYP_BYREF, m_compiler->gtNewLclvNode(newLclNum, TYP_STRUCT));
+                *use = newTree;
+            }
 
-    const unsigned int lclNum    = tree->AsLclVarCommon()->gtLclNum;
-    unsigned int       newLclNum = BAD_VAR_NUM;
-    Compiler*          comp      = allocator->comp;
+            return Compiler::fgWalkResult::WALK_CONTINUE;
+        }
+    };
+
+    BasicBlock* block;
 
-    if (allocator->m_HeapLocalToStackLocalMap.TryGetValue(lclNum, &newLclNum))
+    foreach_block(comp, block)
     {
-        GenTree* newTree = comp->gtNewOperNode(GT_ADDR, TYP_BYREF, comp->gtNewLclvNode(newLclNum, TYP_STRUCT));
-        *pTree           = newTree;
+        for (GenTreeStmt* stmt = block->firstStmt(); stmt; stmt = stmt->gtNextStmt)
+        {
+            RewriteUsesVisitor rewriteUsesVisitor(this);
+            rewriteUsesVisitor.WalkTree(&stmt->gtStmtExpr, nullptr);
+        }
     }
-
-    return Compiler::fgWalkResult::WALK_CONTINUE;
 }
diff --git a/src/jit/objectalloc.h b/src/jit/objectalloc.h
@@ -57,9 +57,7 @@ class ObjectAllocator final : public Phase
     GenTree* MorphAllocObjNodeIntoHelperCall(GenTreeAllocObj* allocObj);
     unsigned int MorphAllocObjNodeIntoStackAlloc(GenTreeAllocObj* allocObj, BasicBlock* block, GenTreeStmt* stmt);
     struct BuildConnGraphVisitorCallbackData;
-    bool CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parentStack, unsigned int lclNum, Compiler* compiler);
-    static Compiler::fgWalkResult BuildConnGraphVisitor(GenTree** pTree, Compiler::fgWalkData* data);
-    static Compiler::fgWalkResult RewriteUsesVisitor(GenTree** pTree, Compiler::fgWalkData* data);
+    bool CanLclVarEscapeViaParentStack(ArrayStack<GenTree*>* parentStack, unsigned int lclNum);
 #ifdef DEBUG
     static Compiler::fgWalkResult AssertWhenAllocObjFoundVisitor(GenTree** pTree, Compiler::fgWalkData* data);
 #endif // DEBUG
@@ -119,13 +117,15 @@ inline bool ObjectAllocator::CanAllocateLclVarOnStack(unsigned int lclNum, CORIN
 {
     assert(m_AnalysisDone);
 
-    if (comp->info.compCompHnd->getClassAttribs(clsHnd) & CORINFO_FLG_CONTAINS_GC_PTR)
+    DWORD classAttribs = comp->info.compCompHnd->getClassAttribs(clsHnd);
+
+    if ((classAttribs & CORINFO_FLG_CONTAINS_GC_PTR) != 0)
     {
         // TODO-ObjectStackAllocation: enable stack allocation of objects with gc fields
         return false;
     }
 
-    if (comp->info.compCompHnd->isValueClass(clsHnd))
+    if ((classAttribs & CORINFO_FLG_VALUECLASS) != 0)
     {
         // TODO-ObjectStackAllocation: enable stack allocation of boxed structs
         return false;
diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp
@@ -1961,6 +1961,10 @@ CEEInfo::getHeapClassSize(
     MethodTable* pMT = VMClsHnd.GetMethodTable();
     _ASSERTE(pMT);
     _ASSERTE(!pMT->IsValueType());
+    _ASSERTE(!pMT->HasComponentSize());
+#ifdef FEATURE_READYTORUN_COMPILER
+    _ASSERTE(!IsReadyToRunCompilation() || pMT->IsInheritanceChainLayoutFixedInCurrentVersionBubble());
+#endif
 
     // Add OBJECT_SIZE to account for method table pointer.
     result = pMT->GetNumInstanceFieldBytes() + OBJECT_SIZE;
@@ -2283,6 +2287,11 @@ unsigned CEEInfo::getClassGClayout (CORINFO_CLASS_HANDLE clsHnd, BYTE* gcPtrs)
         _ASSERTE(sizeof(BYTE) == 1);
 
         BOOL isValueClass = pMT->IsValueType();
+
+#ifdef FEATURE_READYTORUN_COMPILER
+        _ASSERTE(isValueClass || !IsReadyToRunCompilation() || pMT->IsInheritanceChainLayoutFixedInCurrentVersionBubble());
+#endif
+
         unsigned int size = isValueClass ? VMClsHnd.GetSize() : pMT->GetNumInstanceFieldBytes() + OBJECT_SIZE;
 
         // assume no GC pointers at first
diff --git a/tests/issues.targets b/tests/issues.targets
@@ -312,6 +312,13 @@
         </ExcludeList>
     </ItemGroup>
 
+  <!-- x86 specific excludes -->
+  <ItemGroup Condition="'$(XunitTestBinBase)' != '' and '$(BuildArch)' == 'x86'">
+    <ExcludeList Include="$(XunitTestBinBase)/JIT/opt/ObjectStackAllocation/*">
+      <Issue>Object stack allocation is not supported on x86 yet</Issue>
+    </ExcludeList>
+  </ItemGroup>
+
     <!-- Windows arm32 specific excludes -->
     <ItemGroup Condition="'$(XunitTestBinBase)' != '' and '$(BuildArch)' == 'arm' and '$(TargetsWindows)' == 'true'">
         <ExcludeList Include="$(XunitTestBinBase)/Interop/COM/NETClients/Primitives/NETClientPrimitives/*">

Original file line number	Diff line number	Diff line change
`@@ -16734,10 +16734,14 @@ void Compiler::fgMorph()`
`16734`	`16734`	`// local variable allocation on the stack.`
`16735`	`16735`	`ObjectAllocator objectAllocator(this); // PHASE_ALLOCATE_OBJECTS`
`16736`	`16736`
	`16737`	`+// TODO-ObjectStackAllocation: Enable the optimization for architectures using`
	`16738`	`+// JIT32_GCENCODER (i.e., x86).`
	`16739`	`+#ifndef JIT32_GCENCODER`
`16737`	`16740`	`if (JitConfig.JitObjectStackAllocation() && !opts.MinOpts() && !opts.compDbgCode)`
`16738`	`16741`	`{`
`16739`	`16742`	`objectAllocator.EnableObjectStackAllocation();`
`16740`	`16743`	`}`
	`16744`	`+#endif // JIT32_GCENCODER`
`16741`	`16745`
`16742`	`16746`	`objectAllocator.Run();`
`16743`	`16747`