Bug 991720 part 4 - Scalar replacement registers the state instead of replacing resume points operands. r=h4writer

js/src/jit/ScalarReplacement.cpp

@@ -17,42 +17,6 @@
 namespace js {
 namespace jit {
 
-// Scan resume point operands in search of a local variable which captures the
-// current object, and replace it by the current object with its state.
-static void
-ReplaceResumePointOperands(MResumePoint *resumePoint, MDefinition *object, MDefinition *state)
-{
-    // Note: This function iterates over the caller as well, this is wrong
-    // because if the object appears in one of the caller, we want to correctly
-    // recover the object value from any block having the same caller.  In
-    // practice, this is correct for 2 reasons:
-    //
-    // 1. We replace resume point operands in RPO, this implies that the caller
-    // would first be updated when we update the resume point of entry block of
-    // the inner function.  This implies that the object state would only hold
-    // valid data for the caller resume point.
-    //
-    // 2. The caller resume point will have no reference of the new object
-    // allocation if the object allocation is done within the callee.
-    //
-    // A side-effect of this implementation is that we would be restoring and
-    // keeping tracks of the content of the object at the entry of the function,
-    // in addition to the content of the object within the function.
-    for (MResumePoint *rp = resumePoint; rp; rp = rp->caller()) {
-        for (size_t op = 0; op < rp->numOperands(); op++) {
-            if (rp->getOperand(op) == object) {
-                rp->replaceOperand(op, state);
-
-                // This assertion verifies the comment which is above still
-                // holds.  Note, this is not true if rp == resumePoint, as the
-                // object state can be a new one created at the beginning of the
-                // block to keep track of the merge state.
-                MOZ_ASSERT_IF(rp != resumePoint, state->block()->dominates(rp->block()));
-            }
-        }
-    }
-}
-
 template <typename MemoryView>
 class EmulateStateOf
 {
@@ -253,6 +217,9 @@ class ObjectMemoryView : public MDefinitionVisitorDefaultNoop
     MBasicBlock *startBlock_;
     BlockState *state_;
 
+    // Used to improve the memory usage by sharing common modification.
+    const MResumePoint *lastResumePoint_;
+
   public:
     ObjectMemoryView(TempAllocator &alloc, MInstruction *obj);
 
@@ -270,12 +237,14 @@ class ObjectMemoryView : public MDefinitionVisitorDefaultNoop
 
   public:
     void visitResumePoint(MResumePoint *rp);
+    void visitObjectState(MObjectState *ins);
     void visitStoreFixedSlot(MStoreFixedSlot *ins);
     void visitLoadFixedSlot(MLoadFixedSlot *ins);
     void visitStoreSlot(MStoreSlot *ins);
     void visitLoadSlot(MLoadSlot *ins);
     void visitGuardShape(MGuardShape *ins);
     void visitFunctionEnvironment(MFunctionEnvironment *ins);
+    void visitLambda(MLambda *ins);
 };
 
 const char *ObjectMemoryView::phaseName = "Scalar Replacement of Object";
@@ -283,9 +252,14 @@ const char *ObjectMemoryView::phaseName = "Scalar Replacement of Object";
 ObjectMemoryView::ObjectMemoryView(TempAllocator &alloc, MInstruction *obj)
   : alloc_(alloc),
     obj_(obj),
-    startBlock_(obj->block())
+    startBlock_(obj->block()),
+    state_(nullptr),
+    lastResumePoint_(nullptr)
 {
-    // Annoatte the instruction such that we do not replace it by a
+    // Annotate snapshots RValue such that we recover the store first.
+    obj_->setIncompleteObject();
+
+    // Annotate the instruction such that we do not replace it by a
     // Magic(JS_OPTIMIZED_OUT) in case of removed uses.
     obj_->setImplicitlyUsedUnchecked();
 }
@@ -307,6 +281,9 @@ ObjectMemoryView::initStartingState(BlockState **pState)
     BlockState *state = BlockState::New(alloc_, obj_, undefinedVal_);
     startBlock_->insertAfter(obj_, state);
 
+    // Hold out of resume point until it is visited.
+    state->setInWorklist();
+
     *pState = state;
     return true;
 }
@@ -369,7 +346,7 @@ ObjectMemoryView::mergeIntoSuccessorState(MBasicBlock *curr, MBasicBlock *succ,
         // of the successor block, after all the phi nodes.  Note that it
         // would be captured by the entry resume point of the successor
         // block.
-        succ->insertBefore(*succ->begin(), succState);
+        succ->insertBefore(succ->safeInsertTop(), succState);
         *pSuccState = succState;
     }
 
@@ -405,14 +382,13 @@ ObjectMemoryView::assertSuccess()
 {
     for (MUseIterator i(obj_->usesBegin()); i != obj_->usesEnd(); i++) {
         MNode *ins = (*i)->consumer();
+        MDefinition *def = nullptr;
 
         // Resume points have been replaced by the object state.
-        MOZ_ASSERT(!ins->isResumePoint());
-
-        MDefinition *def = ins->toDefinition();
-
-        if (def->isRecoveredOnBailout())
+        if (ins->isResumePoint() || (def = ins->toDefinition())->isRecoveredOnBailout()) {
+            MOZ_ASSERT(obj_->isIncompleteObject());
             continue;
+        }
 
         // The only remaining uses would be removed by DCE, which will also
         // recover the object on bailouts.
@@ -425,7 +401,19 @@ ObjectMemoryView::assertSuccess()
 void
 ObjectMemoryView::visitResumePoint(MResumePoint *rp)
 {
-    ReplaceResumePointOperands(rp, obj_, state_);
+    // As long as the MObjectState is not yet seen next to the allocation, we do
+    // not patch the resume point to recover the side effects.
+    if (!state_->isInWorklist()) {
+        rp->addStore(alloc_, state_, lastResumePoint_);
+        lastResumePoint_ = rp;
+    }
+}
+
+void
+ObjectMemoryView::visitObjectState(MObjectState *ins)
+{
+    if (ins->isInWorklist())
+        ins->setNotInWorklist();
 }
 
 void
@@ -525,6 +513,17 @@ ObjectMemoryView::visitFunctionEnvironment(MFunctionEnvironment *ins)
     ins->block()->discard(ins);
 }
 
+void
+ObjectMemoryView::visitLambda(MLambda *ins)
+{
+    if (ins->scopeChain() != obj_)
+        return;
+
+    // In order to recover the lambda we need to recover the scope chain, as the
+    // lambda is holding it.
+    ins->setIncompleteObject();
+}
+
 static bool
 IndexOf(MDefinition *ins, int32_t *res)
 {
@@ -712,6 +711,9 @@ class ArrayMemoryView : public MDefinitionVisitorDefaultNoop
     MBasicBlock *startBlock_;
     BlockState *state_;
 
+    // Used to improve the memory usage by sharing common modification.
+    const MResumePoint *lastResumePoint_;
+
   public:
     ArrayMemoryView(TempAllocator &alloc, MInstruction *arr);
 
@@ -733,6 +735,7 @@ class ArrayMemoryView : public MDefinitionVisitorDefaultNoop
 
   public:
     void visitResumePoint(MResumePoint *rp);
+    void visitArrayState(MArrayState *ins);
     void visitStoreElement(MStoreElement *ins);
     void visitLoadElement(MLoadElement *ins);
     void visitSetInitializedLength(MSetInitializedLength *ins);
@@ -748,8 +751,12 @@ ArrayMemoryView::ArrayMemoryView(TempAllocator &alloc, MInstruction *arr)
     length_(nullptr),
     arr_(arr),
     startBlock_(arr->block()),
-    state_(nullptr)
+    state_(nullptr),
+    lastResumePoint_(nullptr)
 {
+    // Annotate snapshots RValue such that we recover the store first.
+    arr_->setIncompleteObject();
+
     // Annotate the instruction such that we do not replace it by a
     // Magic(JS_OPTIMIZED_OUT) in case of removed uses.
     arr_->setImplicitlyUsedUnchecked();
@@ -774,6 +781,9 @@ ArrayMemoryView::initStartingState(BlockState **pState)
     BlockState *state = BlockState::New(alloc_, arr_, undefinedVal_, initLength);
     startBlock_->insertAfter(arr_, state);
 
+    // Hold out of resume point until it is visited.
+    state->setInWorklist();
+
     *pState = state;
     return true;
 }
@@ -836,7 +846,7 @@ ArrayMemoryView::mergeIntoSuccessorState(MBasicBlock *curr, MBasicBlock *succ,
         // of the successor block, after all the phi nodes.  Note that it
         // would be captured by the entry resume point of the successor
         // block.
-        succ->insertBefore(*succ->begin(), succState);
+        succ->insertBefore(succ->safeInsertTop(), succState);
         *pSuccState = succState;
     }
 
@@ -877,7 +887,19 @@ ArrayMemoryView::assertSuccess()
 void
 ArrayMemoryView::visitResumePoint(MResumePoint *rp)
 {
-    ReplaceResumePointOperands(rp, arr_, state_);
+    // As long as the MArrayState is not yet seen next to the allocation, we do
+    // not patch the resume point to recover the side effects.
+    if (!state_->isInWorklist()) {
+        rp->addStore(alloc_, state_, lastResumePoint_);
+        lastResumePoint_ = rp;
+    }
+}
+
+void
+ArrayMemoryView::visitArrayState(MArrayState *ins)
+{
+    if (ins->isInWorklist())
+        ins->setNotInWorklist();
 }
 
 bool