[MERGE #5169 @Cellule] ByteCodeUses Aggregation in DeadStore

Cellule · Cellule · commit 012c0e8493fa · 2018-05-16T23:12:21.000-07:00
Merge pull request #5169 from Cellule:bytecode_use Do ByteCodeUsesInstr aggregation in the deadstore pass instead of the forward pass.
diff --git a/lib/Backend/BackwardPass.cpp b/lib/Backend/BackwardPass.cpp
@@ -1996,41 +1996,76 @@ BackwardPass::ProcessBailOutCopyProps(BailOutInfo * bailOutInfo, BVSparse<JitAre
     NEXT_SLISTBASE_ENTRY_EDITING;
 }
 
-bool
-BackwardPass::ProcessBailOutInfo(IR::Instr * instr)
+
+StackSym*
+BackwardPass::ProcessByteCodeUsesDst(IR::ByteCodeUsesInstr * byteCodeUsesInstr)
 {
-    if (this->tag == Js::BackwardPhase)
+    Assert(this->DoByteCodeUpwardExposedUsed());
+    IR::Opnd * dst = byteCodeUsesInstr->GetDst();
+    if (dst)
     {
-        // We don't need to fill in the bailout instruction in backward pass
-        Assert(this->func->hasBailout || !instr->HasBailOutInfo());
-        Assert(!instr->HasBailOutInfo() || instr->GetBailOutInfo()->byteCodeUpwardExposedUsed == nullptr || (this->func->HasTry() && this->func->DoOptimizeTry()));
+        IR::RegOpnd * dstRegOpnd = dst->AsRegOpnd();
+        StackSym * dstStackSym = dstRegOpnd->m_sym->AsStackSym();
+        Assert(!dstRegOpnd->GetIsJITOptimizedReg());
+        Assert(dstStackSym->GetByteCodeRegSlot() != Js::Constants::NoRegister);
+        if (dstStackSym->GetType() != TyVar)
+        {
+            dstStackSym = dstStackSym->GetVarEquivSym(nullptr);
+        }
 
-        if (instr->IsByteCodeUsesInstr())
+        // If the current region is a Try, symbols in its write-through set shouldn't be cleared.
+        // Otherwise, symbols in the write-through set of the first try ancestor shouldn't be cleared.
+        if (!this->currentRegion ||
+            !this->CheckWriteThroughSymInRegion(this->currentRegion, dstStackSym))
         {
-            // FGPeeps inserts bytecodeuses instrs with srcs.  We need to look at them to set the proper
-            // UpwardExposedUsed info and keep the defs alive.
-            // The inliner inserts bytecodeuses instrs withs dsts, but we don't want to look at them for upwardExposedUsed
-            // as it would cause real defs to look dead.  We use these for bytecodeUpwardExposedUsed info only, which is needed
-            // in the dead-store pass only.
-            //
-            // Handle the source side.
-            IR::ByteCodeUsesInstr *byteCodeUsesInstr = instr->AsByteCodeUsesInstr();
-            const BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed = byteCodeUsesInstr->GetByteCodeUpwardExposedUsed();
-            if (byteCodeUpwardExposedUsed != nullptr)
-            {
-                this->currentBlock->upwardExposedUses->Or(byteCodeUpwardExposedUsed);
-                if (this->DoByteCodeUpwardExposedUsed())
-                {
-                    this->currentBlock->byteCodeUpwardExposedUsed->Or(byteCodeUpwardExposedUsed);
-                }
-            }
-            return true;
+            this->currentBlock->byteCodeUpwardExposedUsed->Clear(dstStackSym->m_id);
+            return dstStackSym;
+
         }
+    }
+    return nullptr;
+}
+
+const BVSparse<JitArenaAllocator>*
+BackwardPass::ProcessByteCodeUsesSrcs(IR::ByteCodeUsesInstr * byteCodeUsesInstr)
+{
+    Assert(this->DoByteCodeUpwardExposedUsed() || tag == Js::BackwardPhase);
+    const BVSparse<JitArenaAllocator>* byteCodeUpwardExposedUsed = byteCodeUsesInstr->GetByteCodeUpwardExposedUsed();
+    if (byteCodeUpwardExposedUsed && this->DoByteCodeUpwardExposedUsed())
+    {
+        this->currentBlock->byteCodeUpwardExposedUsed->Or(byteCodeUpwardExposedUsed);
+    }
+    return byteCodeUpwardExposedUsed;
+}
+
+bool
+BackwardPass::ProcessByteCodeUsesInstr(IR::Instr * instr)
+{
+    if (!instr->IsByteCodeUsesInstr())
+    {
         return false;
     }
 
-    if (instr->IsByteCodeUsesInstr())
+    IR::ByteCodeUsesInstr * byteCodeUsesInstr = instr->AsByteCodeUsesInstr();
+
+    if (this->tag == Js::BackwardPhase)
     {
+        // FGPeeps inserts bytecodeuses instrs with srcs.  We need to look at them to set the proper
+        // UpwardExposedUsed info and keep the defs alive.
+        // The inliner inserts bytecodeuses instrs withs dsts, but we don't want to look at them for upwardExposedUsed
+        // as it would cause real defs to look dead.  We use these for bytecodeUpwardExposedUsed info only, which is needed
+        // in the dead-store pass only.
+        //
+        // Handle the source side.
+        const BVSparse<JitArenaAllocator>* byteCodeUpwardExposedUsed = ProcessByteCodeUsesSrcs(byteCodeUsesInstr);
+        if (byteCodeUpwardExposedUsed != nullptr)
+        {
+            this->currentBlock->upwardExposedUses->Or(byteCodeUpwardExposedUsed);
+        }
+    }
+    else
+    {
+        Assert(tag == Js::DeadStorePhase);
         Assert(instr->m_opcode == Js::OpCode::ByteCodeUses);
 #if DBG
         if (this->DoMarkTempObjectVerify() && (this->currentBlock->isDead || !this->func->hasBailout))
@@ -2053,44 +2088,22 @@ BackwardPass::ProcessBailOutInfo(IR::Instr * instr)
 
         if (this->func->hasBailout)
         {
-            Assert(this->DoByteCodeUpwardExposedUsed());
-
             // Just collect the byte code uses, and remove the instruction
             // We are going backward, process the dst first and then the src
-            IR::Opnd * dst = instr->GetDst();
-            if (dst)
-            {
-                IR::RegOpnd * dstRegOpnd = dst->AsRegOpnd();
-                StackSym * dstStackSym = dstRegOpnd->m_sym->AsStackSym();
-                Assert(!dstRegOpnd->GetIsJITOptimizedReg());
-                Assert(dstStackSym->GetByteCodeRegSlot() != Js::Constants::NoRegister);
-                if (dstStackSym->GetType() != TyVar)
-                {
-                    dstStackSym = dstStackSym->GetVarEquivSym(nullptr);
-                }
-
-                // If the current region is a Try, symbols in its write-through set shouldn't be cleared.
-                // Otherwise, symbols in the write-through set of the first try ancestor shouldn't be cleared.
-                if (!this->currentRegion ||
-                    !this->CheckWriteThroughSymInRegion(this->currentRegion, dstStackSym))
-                {
-                    this->currentBlock->byteCodeUpwardExposedUsed->Clear(dstStackSym->m_id);
+            StackSym *dstStackSym = ProcessByteCodeUsesDst(byteCodeUsesInstr);
 #if DBG
-                    // We can only track first level function stack syms right now
-                    if (dstStackSym->GetByteCodeFunc() == this->func)
-                    {
-                        this->currentBlock->byteCodeRestoreSyms[dstStackSym->GetByteCodeRegSlot()] = nullptr;
-                    }
-#endif
-                }
+            // We can only track first level function stack syms right now
+            if (dstStackSym && dstStackSym->GetByteCodeFunc() == this->func)
+            {
+                this->currentBlock->byteCodeRestoreSyms[dstStackSym->GetByteCodeRegSlot()] = nullptr;
             }
+#endif
 
-            IR::ByteCodeUsesInstr *byteCodeUsesInstr = instr->AsByteCodeUsesInstr();
-            if (byteCodeUsesInstr->GetByteCodeUpwardExposedUsed() != nullptr)
-            {
-                this->currentBlock->byteCodeUpwardExposedUsed->Or(byteCodeUsesInstr->GetByteCodeUpwardExposedUsed());
+            const BVSparse<JitArenaAllocator>* byteCodeUpwardExposedUsed = ProcessByteCodeUsesSrcs(byteCodeUsesInstr);
 #if DBG
-                FOREACH_BITSET_IN_SPARSEBV(symId, byteCodeUsesInstr->GetByteCodeUpwardExposedUsed())
+            if (byteCodeUpwardExposedUsed)
+            {
+                FOREACH_BITSET_IN_SPARSEBV(symId, byteCodeUpwardExposedUsed)
                 {
                     StackSym * stackSym = this->func->m_symTable->FindStackSym(symId);
                     Assert(!stackSym->IsTypeSpec());
@@ -2108,16 +2121,14 @@ BackwardPass::ProcessBailOutInfo(IR::Instr * instr)
                     }
                 }
                 NEXT_BITSET_IN_SPARSEBV;
-#endif
             }
+#endif
 
-            if(IsCollectionPass())
+            if (IsCollectionPass())
             {
                 return true;
             }
 
-            ProcessPendingPreOpBailOutInfo(instr);
-
             PropertySym *propertySymUse = byteCodeUsesInstr->propertySymUse;
             if (propertySymUse && !this->currentBlock->isDead)
             {
@@ -2133,13 +2144,27 @@ BackwardPass::ProcessBailOutInfo(IR::Instr * instr)
         }
 
         this->currentBlock->RemoveInstr(instr);
-        return true;
+    }
+    return true;
+}
+
+bool
+BackwardPass::ProcessBailOutInfo(IR::Instr * instr)
+{
+    Assert(!instr->IsByteCodeUsesInstr());
+    if (this->tag == Js::BackwardPhase)
+    {
+        // We don't need to fill in the bailout instruction in backward pass
+        Assert(this->func->hasBailout || !instr->HasBailOutInfo());
+        Assert(!instr->HasBailOutInfo() || instr->GetBailOutInfo()->byteCodeUpwardExposedUsed == nullptr || (this->func->HasTry() && this->func->DoOptimizeTry()));
+        return false;
     }
 
     if(IsCollectionPass())
     {
         return false;
     }
+    Assert(tag == Js::DeadStorePhase);
 
     if (instr->HasBailOutInfo())
     {
@@ -2398,22 +2423,30 @@ BackwardPass::NeedBailOutOnImplicitCallsForTypedArrayStore(IR::Instr* instr)
     return false;
 }
 
-void
+IR::Instr*
 BackwardPass::ProcessPendingPreOpBailOutInfo(IR::Instr *const currentInstr)
 {
     Assert(!IsCollectionPass());
 
     if(!preOpBailOutInstrToProcess)
     {
-        return;
+        return currentInstr->m_prev;
     }
+    Assert(preOpBailOutInstrToProcess == currentInstr);
 
-    IR::Instr *const prevInstr = currentInstr->m_prev;
-    if(prevInstr &&
-        prevInstr->IsByteCodeUsesInstr() &&
-        prevInstr->AsByteCodeUsesInstr()->GetByteCodeOffset() == preOpBailOutInstrToProcess->GetByteCodeOffset())
+    if (!this->IsPrePass())
     {
-        return;
+        IR::Instr* prev = preOpBailOutInstrToProcess->m_prev;
+        while (prev && preOpBailOutInstrToProcess->CanAggregateByteCodeUsesAcrossInstr(prev))
+        {
+            IR::Instr* instr = prev;
+            prev = prev->m_prev;
+            if (instr->IsByteCodeUsesInstrFor(preOpBailOutInstrToProcess))
+            {
+                // If instr is a ByteCodeUsesInstr, it will remove it
+                ProcessByteCodeUsesInstr(instr);
+            }
+        }
     }
 
     // A pre-op bailout instruction was saved for bailout info processing after the instruction and relevant ByteCodeUses
@@ -2423,6 +2456,10 @@ BackwardPass::ProcessPendingPreOpBailOutInfo(IR::Instr *const currentInstr)
     Assert(bailOutInfo->bailOutOffset == preOpBailOutInstrToProcess->GetByteCodeOffset());
     ProcessBailOutInfo(preOpBailOutInstrToProcess, bailOutInfo);
     preOpBailOutInstrToProcess = nullptr;
+
+    // We might have removed the prev instr if it was a ByteCodeUsesInstr
+    // Update the prevInstr on the main loop
+    return currentInstr->m_prev;
 }
 
 void
@@ -2720,7 +2757,7 @@ BackwardPass::ProcessBlock(BasicBlock * block)
         MarkScopeObjSymUseForStackArgOpt();
         ProcessBailOnStackArgsOutOfActualsRange();
 
-        if (ProcessNoImplicitCallUses(instr) || this->ProcessBailOutInfo(instr))
+        if (ProcessNoImplicitCallUses(instr) || this->ProcessByteCodeUsesInstr(instr) || this->ProcessBailOutInfo(instr))
         {
             continue;
         }
@@ -3531,7 +3568,7 @@ BackwardPass::ProcessBlock(BasicBlock * block)
             }
         }
 #endif
-        ProcessPendingPreOpBailOutInfo(instr);
+        instrPrev = ProcessPendingPreOpBailOutInfo(instr);
 
 #if DBG_DUMP
         if (!IsCollectionPass() && IsTraceEnabled() && Js::Configuration::Global.flags.Verbose)
diff --git a/lib/Backend/BackwardPass.h b/lib/Backend/BackwardPass.h
@@ -37,9 +37,12 @@ class BackwardPass
     void ProcessTransfers(IR::Instr * instr);
     void ProcessFieldKills(IR::Instr * instr);
     template<typename T> void ClearBucketsOnFieldKill(IR::Instr *instr, HashTable<T> *table);
+    StackSym* ProcessByteCodeUsesDst(IR::ByteCodeUsesInstr * byteCodeUsesInstr);
+    const BVSparse<JitArenaAllocator>* ProcessByteCodeUsesSrcs(IR::ByteCodeUsesInstr * byteCodeUsesInstr);
+    bool ProcessByteCodeUsesInstr(IR::Instr * instr);
     bool ProcessBailOutInfo(IR::Instr * instr);
     void ProcessBailOutInfo(IR::Instr * instr, BailOutInfo * bailOutInfo);
-    void ProcessPendingPreOpBailOutInfo(IR::Instr *const currentInstr);
+    IR::Instr* ProcessPendingPreOpBailOutInfo(IR::Instr *const currentInstr);
     void ProcessBailOutArgObj(BailOutInfo * bailOutInfo, BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed);
     void ProcessBailOutConstants(BailOutInfo * bailOutInfo, BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed, BVSparse<JitArenaAllocator>* argSymsBv);
     void ProcessBailOutCopyProps(BailOutInfo * bailOutInfo, BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed, BVSparse<JitArenaAllocator>* argSymsBv);
diff --git a/lib/Backend/GlobOpt.cpp b/lib/Backend/GlobOpt.cpp
@@ -2670,16 +2670,6 @@ GlobOpt::OptInstr(IR::Instr *&instr, bool* isInstrRemoved)
         this->InsertByteCodeUses(instr);
     }
 
-    if (!IsLoopPrePass() && instr->HasBailOutInfo() && !this->func->IsJitInDebugMode() && !PHASE_OFF(Js::AggregateByteCodeUsesPhase, this->func))
-    {
-        // Aggregate byteCodeUpwardExposedUsed of preceding ByteCodeUses instrs with the same bytecode offset.
-        // This is required as different ByteCodeUses instrs may be inserted for an instr in the loop pre-pass
-        // and the main pass (and there may be additional instructions inserted between the two sets of 
-        // ByteCodeUses instructions), but the Backward Pass only processes immediately preceding consecutive
-        // ByteCodeUses instructions before processing a pre-op bailout.
-        instr->AggregateByteCodeUses();
-    }
-
     if (!this->IsLoopPrePass() && !isHoisted && this->IsImplicitCallBailOutCurrentlyNeeded(instr, src1Val, src2Val))
     {
         IR::BailOutKind kind = IR::BailOutOnImplicitCalls;
diff --git a/lib/Backend/GlobOptExpr.cpp b/lib/Backend/GlobOptExpr.cpp
@@ -798,6 +798,7 @@ GlobOpt::CSEOptimize(BasicBlock *block, IR::Instr * *const instrRef, Value **pSr
         // code and due to other similar potential issues, always create a new instr instead of changing the existing one.
         IR::Instr *const originalInstr = instr;
         instr = IR::Instr::New(Js::OpCode::Ld_A, instr->GetDst(), cseOpnd, instr->m_func);
+        instr->SetByteCodeOffset(originalInstr);
         originalInstr->TransferDstAttributesTo(instr);
         block->InsertInstrBefore(instr, originalInstr);
         block->RemoveInstr(originalInstr);
diff --git a/lib/Backend/IR.cpp b/lib/Backend/IR.cpp
@@ -1020,6 +1020,7 @@ void ByteCodeUsesInstr::Aggregate(ByteCodeUsesInstr * byteCodeUsesInstr)
     Assert(this->m_func == byteCodeUsesInstr->m_func);
     if (byteCodeUsesInstr->byteCodeUpwardExposedUsed)
     {
+        Assert(byteCodeUsesInstr->GetDst() == nullptr);
         if (this->byteCodeUpwardExposedUsed)
         {
             this->byteCodeUpwardExposedUsed->Or(byteCodeUsesInstr->byteCodeUpwardExposedUsed);
@@ -1036,7 +1037,7 @@ void ByteCodeUsesInstr::Aggregate(ByteCodeUsesInstr * byteCodeUsesInstr)
 
 bool Instr::CanAggregateByteCodeUsesAcrossInstr(Instr * instr)
 {
-    return !instr->IsBranchInstr() && 
+    return !instr->EndsBasicBlock() && 
         instr->m_func == this->m_func &&
         ((instr->GetByteCodeOffset() == Js::Constants::NoByteCodeOffset) ||
         (instr->GetByteCodeOffset() == this->GetByteCodeOffset()));
@@ -2871,21 +2872,6 @@ IR::Instr *Instr::GetInsertBeforeByteCodeUsesInstr()
     return insertBeforeInstr;
 }
 
-IR::ByteCodeUsesInstr *
-Instr::GetFirstByteCodeUsesInstrBackward()
-{
-    IR::Instr * prevInstr = this->m_prev;
-    while(prevInstr && CanAggregateByteCodeUsesAcrossInstr(prevInstr))
-    {
-        if (prevInstr->IsByteCodeUsesInstr() && prevInstr->GetByteCodeOffset() == this->GetByteCodeOffset())
-        {
-            return prevInstr->AsByteCodeUsesInstr();
-        }
-        prevInstr = prevInstr->m_prev;
-    }
-    return nullptr;
-}
-
 bool
 Instr::IsByteCodeUsesInstrFor(IR::Instr * instr) const
 {
@@ -3395,22 +3381,6 @@ void Instr::Move(IR::Instr* insertInstr)
     insertInstr->InsertBefore(this);
 }
 
-void Instr::AggregateByteCodeUses()
-{
-    // Currently, this only aggregates byteCodeUpwardExposedUsed of those ByteCodeUses instructions 
-    // associated with this instr which have at most a ToVar, Ld_A, Ld_I4, or a BailOnNotObject between them.
-    IR::ByteCodeUsesInstr * primaryByteCodeUsesInstr = this->GetFirstByteCodeUsesInstrBackward();
-    if (primaryByteCodeUsesInstr)
-    {
-        primaryByteCodeUsesInstr->AggregatePrecedingByteCodeUses();
-        if (primaryByteCodeUsesInstr != this->m_prev)
-        {
-            primaryByteCodeUsesInstr->Unlink();
-            this->InsertBefore(primaryByteCodeUsesInstr);
-        }
-    }
-}
-
 IR::Instr* Instr::GetBytecodeArgOutCapture()
 {
     Assert(this->m_opcode == Js::OpCode::ArgOut_A_Inline ||
diff --git a/lib/Backend/IR.h b/lib/Backend/IR.h
@@ -296,7 +296,6 @@ class Instr
     IR::Instr *     GetPrevRealInstr() const;
     IR::Instr *     GetPrevRealInstrOrLabel() const;
     IR::Instr *     GetInsertBeforeByteCodeUsesInstr();
-    IR::ByteCodeUsesInstr * GetFirstByteCodeUsesInstrBackward();
     bool            IsByteCodeUsesInstrFor(IR::Instr * instr) const;
     IR::LabelInstr *GetOrCreateContinueLabel(const bool isHelper = false);
     static bool     HasSymUseSrc(StackSym *sym, IR::Opnd*);
@@ -475,7 +474,6 @@ class Instr
     bool       UsesAllFields();
     void       MoveArgs(bool generateByteCodeCapture = false);
     void       Move(IR::Instr* insertInstr);
-    void       AggregateByteCodeUses();
 private:
     void            ClearNumber() { this->m_number = 0; }
     void            SetNumber(uint32 number);
diff --git a/lib/Common/ConfigFlagsList.h b/lib/Common/ConfigFlagsList.h
@@ -119,7 +119,6 @@ PHASE(All)
                 PHASE(ValueTable)
                 PHASE(ValueNumbering)
                 PHASE(AVTInPrePass)
-                PHASE(AggregateByteCodeUses)
                 PHASE(PathDependentValues)
                     PHASE(TrackRelativeIntBounds)
                         PHASE(BoundCheckElimination)
