Work towards objects stack allocation #6653

Merged
merged 1 commit into from Aug 9, 2016

Projects

None yet

6 participants

@echesakov
Contributor

Work towards objects stack allocation: moved allocation part of newobj-lowering into separate phase

  1. Introduced GT_ALLOCOBJ node to mark places where object allocation happens
  2. In importer.cpp changed lowering of allocation part of newobj instruction from an allocation helper call to a GT_ALLOCOBJ node creation
  3. Created new phase ObjectAllocator (PHASE_ALLOCATE_OBJECTS) and put it right after dominator computing (we will need the information for escape analysis)
  4. Current implementation of ObjectAllocator walks over all basic blocks having flag BBF_HAS_NEWOBJ set and replaces GT_ALLOCOBJ with an allocation helper call

I did coreclr testing and throughput measurement. We have one assembly diff (an improvement) related to deleting unreachable basic blocks. Throughput remains the same.

@erozenfeld PTAL
cc @dotnet/jit-contrib

@CarolEidt CarolEidt commented on the diff Aug 8, 2016
src/jit/importer.cpp
@@ -11852,9 +11852,8 @@ MATH_MAYBE_CALL_NO_OVF: ovfl = false;
// 3) Allocate and return the new object
// Reason: performance (today, we'll always use the slow helper for the R2R generics case)
- op1 = gtNewHelperCallNode( info.compCompHnd->getNewHelper(&resolvedToken, info.compMethodHnd),
- TYP_REF, 0,
- gtNewArgList(op1));
+ op1 = gtNewAllocObjNode( info.compCompHnd->getNewHelper(&resolvedToken, info.compMethodHnd),
+ resolvedToken.hClass, TYP_REF, op1 );
@CarolEidt
CarolEidt Aug 8, 2016 Member

This code always assigns the newly allocated object to a temp. However, it's not entirely clear that this is a fundamental requirement. So, either here or above the call to impAssignTempGen, I would add a comment that the ObjectAllocator relies on this.

@CarolEidt
Member

LGTM, other than one suggestion for a comment.
I think it would be good to add a COMPlus_EnableObjectStackAllocation or similar.

@pgavlin pgavlin commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.cpp
+ return allocObj;
+}
+
+#ifdef DEBUG
+
+//------------------------------------------------------------------------
+// AssertWhenAllocObjFoundVisitor: Looks for a GT_ALLOCOBJ node and assert
+// when found one.
+Compiler::fgWalkResult ObjectAllocator::AssertWhenAllocObjFoundVisitor(GenTreePtr* pTree, Compiler::fgWalkData* data)
+{
+ GenTreePtr tree = *pTree;
+
+ assert(tree != nullptr);
+ assert(tree->OperGet() != GT_ALLOCOBJ);
+
+ return Compiler::fgWalkResult::WALK_CONTINUE;
@pgavlin
pgavlin Aug 8, 2016 Contributor

Looks like the indentation is off in this function.

@pgavlin pgavlin commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.h
+
+class ObjectAllocator : public Phase
+{
+ //===============================================================================
+ // Data members
+ bool m_objectStackAllocationEnabled;
+ bool m_analysisDone;
+ //===============================================================================
+ // Methods
+public:
+ ObjectAllocator(Compiler* comp);
+ bool ObjectStackAllocationEnabled() const;
+ void EnableObjectStackAllocation();
+
+protected:
+ virtual void DoPhase();
@pgavlin
pgavlin Aug 8, 2016 Contributor

nit: virtual void DoPhase() override;

@pgavlin pgavlin commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.h
+XX XX
+XX ObjectAllocator XX
+XX XX
+XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
+XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
+*/
+
+/*****************************************************************************/
+#ifndef OBJECTALLOC_H
+#define OBJECTALLOC_H
+/*****************************************************************************/
+
+//===============================================================================
+#include "phase.h"
+
+class ObjectAllocator : public Phase
@pgavlin
pgavlin Aug 8, 2016 Contributor

Nit: class ObjectAllocator final : public Phase

@pgavlin pgavlin commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.h
+ static Compiler::fgWalkResult AssertWhenAllocObjFoundVisitor(GenTreePtr* pTree, Compiler::fgWalkData* data);
+#endif // DEBUG
+};
+
+//===============================================================================
+
+inline
+ObjectAllocator::ObjectAllocator(Compiler* comp) :
+ Phase(comp, "Allocate Objects", PHASE_ALLOCATE_OBJECTS),
+ m_objectStackAllocationEnabled(false),
+ m_analysisDone(false)
+{
+}
+
+inline
+bool ObjectAllocator::ObjectStackAllocationEnabled() const
@pgavlin
pgavlin Aug 8, 2016 Contributor

Nit: s/ObjectStackAllocationEnabled/IsObjectStackAllocationEnabled/

@pgavlin
pgavlin Aug 8, 2016 Contributor

(same goes for the member field)

@pgavlin
Contributor
pgavlin commented Aug 8, 2016

LGTM aside from a few nits.

@erozenfeld
Member

@dotnet-bot test OSX x64 Checked Build and Test

@erozenfeld
Member

@dotnet-bot test Ubuntu x64 Checked Build and Test

@erozenfeld
Member

@dotnet-bot test Ubuntu x64 Checked Build and Test

@briansull briansull commented on an outdated diff Aug 8, 2016
src/jit/gentree.h
@@ -4221,6 +4221,26 @@ struct GenTreeCopyOrReload : public GenTreeUnOp
#endif
};
+// Represents GT_ALLOCOBJ node
+
+struct GenTreeAllocObj final : public GenTreeUnOp
+{
+ unsigned int gtNewHelper; // Value returned by ICorJitInfo::getNewHelper
+ CORINFO_CLASS_HANDLE gtAllocObjClsHnd;
+
+ GenTreeAllocObj(var_types type, unsigned int helper, CORINFO_CLASS_HANDLE clsHnd, GenTreePtr op
+ DEBUGARG(bool largeNode = false)) :
@briansull
briansull Aug 8, 2016 Contributor

Do we really need another DEBUGARG() here?

PeterK is currently revisiting all uses of "largeNodes" in our codebase.

It would seem that we should know if this node needs to be large or not and not have every callsite determine this properly.

@briansull briansull commented on an outdated diff Aug 8, 2016
src/jit/compphases.h
@@ -31,6 +31,7 @@ CompPhaseNameMacro(PHASE_COMPUTE_EDGE_WEIGHTS, "Compute edge weights (1)",
CompPhaseNameMacro(PHASE_CREATE_FUNCLETS, "Create EH funclets", "EH-FUNC", false, -1)
#endif // FEATURE_EH_FUNCLETS
CompPhaseNameMacro(PHASE_OPTIMIZE_LAYOUT, "Optimize layout", "LAYOUT", false, -1)
+CompPhaseNameMacro(PHASE_ALLOCATE_OBJECTS, "Allocate Objects", "OBJALLOC", false, -1)
@briansull
briansull Aug 8, 2016 Contributor

"OBJALLOC" should be "ALLOC-OBJ" to better match the rest of the code.

@briansull briansull commented on an outdated diff Aug 8, 2016
src/jit/gentree.cpp
@@ -6754,6 +6759,14 @@ GenTreePtr Compiler::gtCloneExpr(GenTree * tree,
}
break;
+ case GT_ALLOCOBJ:
+ {
+ GenTreeAllocObj* asAllocObj = tree->AsAllocObj();
+ copy = new (this, GT_ALLOCOBJ) GenTreeAllocObj(tree->TypeGet(), asAllocObj->gtNewHelper, asAllocObj->gtAllocObjClsHnd, asAllocObj->gtOp1
+ DEBUGARG(/*largeNode*/TRUE));
@briansull
briansull Aug 8, 2016 Contributor

I'm not a fan of conditional arguments.

@erozenfeld erozenfeld commented on an outdated diff Aug 8, 2016
src/jit/importer.cpp
block->bbFlags |= BBF_HAS_NEWOBJ;
optMethodFlags |= OMF_HAS_NEWOBJ;
- /* Append the assignment to the temp/local. Dont need to spill
- at all as we are just calling an EE-Jit helper which can only
- cause an (async) OutOfMemoryException */
+ // Append the assignment to the temp/local. Dont need to spill
+ // at all as we are just calling an EE-Jit helper which can only
+ // cause an (async) OutOfMemoryException.
+
+ // We assign the newly allocated object (by a GT_ALLOCOBJ node)
+ // to a temp. Note that the pattern "temp = allocObj" is required
+ // by ObjectAllocator phase to be able to determine GT_ALLOCOBJ nodes
+ // without exhausted walk over all expressions.
@erozenfeld
erozenfeld Aug 8, 2016 Member

exhausted --> exhaustive

@erozenfeld erozenfeld commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.cpp
+ {
+ return;
+ }
+
+ if (IsObjectStackAllocationEnabled())
+ {
+ DoAnalysis();
+ }
+
+ MorphAllocObjNodes();
+}
+
+//------------------------------------------------------------------------
+// DoAnalysis: Walks over basic blocks of the method and detect all local
+// variables that that can be allocated on the stack.
+//
@erozenfeld
erozenfeld Aug 8, 2016 Member

Walks --> walk

@erozenfeld
erozenfeld Aug 8, 2016 Member

Remove the extra 'that'

@erozenfeld erozenfeld commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.cpp
+ {
+ op2 = MorphAllocObjNodeIntoStackAlloc(asAllocObj, block, stmt);
+ }
+ else
+ {
+ op2 = MorphAllocObjNodeIntoHelperCall(asAllocObj);
+ }
+
+ // Propagate flags of op2 to its parent.
+ stmtExpr->gtOp.gtOp2 = op2;
+ stmtExpr->gtFlags |= op2->gtFlags & GTF_ALL_EFFECT;
+ }
+#ifdef DEBUG
+ else
+ {
+ // We assume that GT_ALLOCOBJ nodes always present in the
@erozenfeld
erozenfeld Aug 8, 2016 Member

always present --> are always present

@erozenfeld erozenfeld commented on an outdated diff Aug 8, 2016
src/jit/objectalloc.cpp
+GenTreePtr ObjectAllocator::MorphAllocObjNodeIntoStackAlloc(GenTreeAllocObj* allocObj, BasicBlock* block, GenTreeStmt* stmt)
+{
+ assert(allocObj != nullptr);
+ assert(m_AnalysisDone);
+
+ // TODO-StackAllocation
+ NYI("MorphAllocObjIntoStackAlloc");
+
+ return allocObj;
+}
+
+#ifdef DEBUG
+
+//------------------------------------------------------------------------
+// AssertWhenAllocObjFoundVisitor: Looks for a GT_ALLOCOBJ node and assert
+// when found one.
@erozenfeld
erozenfeld Aug 8, 2016 Member

Looks --> Look

Egor Chesakov Work towards objects stack allocation: moved allocation part of newob…
…j-lowering into separate phase

1. Introduced `GT_ALLOCOBJ` node to mark places where object allocation happens
2. In `importer.cpp` changed lowering of allocation part of newobj instruction from an allocation helper call to a `GT_ALLOCOBJ` node creation
3. Created new phase `ObjectAllocator` (`PHASE_ALLOCATE_OBJECTS`) and put it right after dominator computing (we will need the information for escape analysis)
4. Current implementation of ObjectAllocator walks over all basic blocks having flag `BBF_HAS_NEWOBJ` set and replaces `GT_ALLOCOBJ` with  an allocation helper call
3c30aa1
@echesakov
Contributor

@dotnet-bot test Windows_NT x86 ryujit Checked Build and Test

@briansull
Contributor

LGTM

@erozenfeld
Member

LGTM

@echesakov echesakov merged commit 3779842 into dotnet:master Aug 9, 2016

10 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 legacy_backend Checked Build and Test Build finished.
Details
Windows_NT x86 ryujit Checked Build and Test Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment