Enabling Stack Args when there is presence of nested functions.

Premise:
The initial implementation of Stack Args was disabled when there is any
nested function.
We were missing a bunch of opportunities in the real world web pages - where nested function is a common thing.
This PR enables this optimization for nested function.

Summary:
Stack Args will work partially when nested functions are present. We will
attempt to remove only Heap Arguments Object creation instr and retain
scope object creation instr (whenever required).

Details:
ByteCodeGeneration:
  - We track if there are any params being closure captured by any nested functions
  - We give up the optimization if this is case.
  - We track if there are any non-local references inside the nested
    function.
  - We do Stack Args partially here - i.e. we attempt to remove Heap Arguments Object creation but still create scope object - as scope
      object is required for FrameDisplay.
IRBuilder:
  - Scope object tracking has been moved from Forward pass (Globopt) to this phase.
  - This enables us to mark the scope obj sym to be live during the
	  first backward pass.
Backward pass:
  - Scope Object Sym is kept alive in all code paths.
  - This is to facilitate Bailout to record the live Scope object Sym, whenever required.
  - Reason for doing is this because - Scope object has to be implicitly live whenever Heap Arguments object is live.
  - When we restore HeapArguments object in the bail out path, it expects the scope object also to be restored - if one was created.
  - We do not know detailed information about Heap arguments obj syms(aliasing etc.) until we complete Forward Pass.
  - And we want to avoid dead sym clean up (in this case, scope object though not explicitly live, it is live implicitly) during Block merging in the forward pass.
Globopt pass:
  - Extra tracking for instrs having arguments object is done.
Dead Store pass:
    - Scope obj creation instruction is not dead stored - It will be a MOV NULL instr in the Lowerer.
    - All instrs related cached scope obj is deadstored. Reason is to dead
      store the cached scope object creation instruction itself.
    - InitCachedFuncs is deadstored.
    - GetCachedFunc is replaced with NewScFunc instr.
Lowerer pass:
    - If Stack Args optimization is still turned on and the parser has hinted that Scope obj creation is not required, then scope object creation instruction will be turned to a MOV NULL.
    - We retain the instruction and not remove it because we want the value of the scope object to be restored
    - That's when we can always rely on the bail out path to restore some
      valid value (NULL or the scope object itself).
BailOut path:
    - We use the restored scope object value (we track this by using a flag on the bailoutInfo) and copy the param values to it.

Added UT and some more traces
Fixing a bug with the Type of Scope object. Type wasn't being transitioned
correctly in cases where we remove LdHeapArguments opcode.
Took care of review comments.

Review Comments

Author: satheeshravi

lib/Backend/BackwardPass.cpp

@@ -295,6 +295,23 @@ BackwardPass::InsertArgInsForFormals()
     }
 }
 
+void
+BackwardPass::MarkScopeObjSymUseForStackArgOpt()
+{
+    IR::Instr * instr = this->currentInstr;
+    if (tag == Js::DeadStorePhase)
+    {
+        if (instr->DoStackArgsOpt(this->func) && instr->m_func->GetScopeObjSym() != nullptr)
+        {
+            if (this->currentBlock->byteCodeUpwardExposedUsed == nullptr)
+            {
+                this->currentBlock->byteCodeUpwardExposedUsed = JitAnew(this->tempAlloc, BVSparse<JitArenaAllocator>, this->tempAlloc);
+            }
+            this->currentBlock->byteCodeUpwardExposedUsed->Set(instr->m_func->GetScopeObjSym()->m_id);
+        }
+    }
+}
+
 void
 BackwardPass::ProcessBailOnStackArgsOutOfActualsRange()
 {
@@ -1912,17 +1929,6 @@ BackwardPass::ProcessBailOutInfo(IR::Instr * instr)
             BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed = byteCodeUsesInstr->byteCodeUpwardExposedUsed;
             if (byteCodeUpwardExposedUsed != nullptr)
             {
-                //Stack Args for Formals optimization.
-                //We will restore the scope object in the bail out path, when we restore Heap arguments object.
-                //So we don't to mark the sym for scope object separately for restoration.
-                //When StackArgs for formal opt is ON , Scope object sym is used by formals access only, which will be replaced by ArgIns and Ld_A 
-                //So it is ok to clear the bit here.
-                //Clearing the bit in byteCodeUpwardExposedUsed here.
-                if (instr->m_func->IsStackArgsEnabled() && instr->m_func->GetScopeObjSym())
-                {
-                    byteCodeUpwardExposedUsed->Clear(instr->m_func->GetScopeObjSym()->m_id);
-                }
-
                 this->currentBlock->byteCodeUpwardExposedUsed->Or(byteCodeUpwardExposedUsed);
 #if DBG
                 FOREACH_BITSET_IN_SPARSEBV(symId, byteCodeUpwardExposedUsed)
@@ -2540,6 +2546,7 @@ BackwardPass::ProcessBlock(BasicBlock * block)
             continue;
         }
 
+        MarkScopeObjSymUseForStackArgOpt();
         ProcessBailOnStackArgsOutOfActualsRange();
 
         if (ProcessNoImplicitCallUses(instr) || this->ProcessBailOutInfo(instr))
@@ -2554,7 +2561,7 @@ BackwardPass::ProcessBlock(BasicBlock * block)
             continue;
         }
 
-        if (CanDeadStoreInstrForScopeObjRemoval() && DeadStoreOrChangeInstrForScopeObjRemoval())
+        if (CanDeadStoreInstrForScopeObjRemoval() && DeadStoreOrChangeInstrForScopeObjRemoval(&instrPrev))
         {
             continue;
         }
@@ -2636,7 +2643,7 @@ BackwardPass::ProcessBlock(BasicBlock * block)
             {
                 case Js::OpCode::LdSlot:
                 {
-                    DeadStoreOrChangeInstrForScopeObjRemoval();
+                    DeadStoreOrChangeInstrForScopeObjRemoval(&instrPrev);
                     break;
                 }
                 case Js::OpCode::InlineArrayPush:
@@ -2889,19 +2896,17 @@ BackwardPass::CanDeadStoreInstrForScopeObjRemoval(Sym *sym) const
     if (tag == Js::DeadStorePhase && this->currentInstr->m_func->IsStackArgsEnabled())
     {
         Func * currFunc = this->currentInstr->m_func;
+        bool doScopeObjCreation = currFunc->GetJITFunctionBody()->GetDoScopeObjectCreation();
         switch (this->currentInstr->m_opcode)
         {
-            case Js::OpCode::LdHeapArguments:
-            case Js::OpCode::LdHeapArgsCached:
-            case Js::OpCode::LdLetHeapArguments:
-            case Js::OpCode::LdLetHeapArgsCached:
+            case Js::OpCode::InitCachedScope:
             {
-                if (this->currentInstr->GetSrc1()->IsScopeObjOpnd(currFunc))
+                if(!doScopeObjCreation && this->currentInstr->GetDst()->IsScopeObjOpnd(currFunc))
                 {
                     /*
-                    *   We don't really dead store these instructions. We just want the source sym of these instructions (which is the scope object)
+                    *   We don't really dead store this instruction. We just want the source sym of this instruction
                     *   to NOT be tracked as USED by this instruction.
-                    *   In case of LdXXHeapArgsXXX opcodes, they will effectively be lowered to dest = MOV NULL, in the lowerer phase.
+                    *   This instr will effectively be lowered to dest = MOV NULLObject, in the lowerer phase.
                     */
                     return true;
                 }
@@ -2916,13 +2921,23 @@ BackwardPass::CanDeadStoreInstrForScopeObjRemoval(Sym *sym) const
                 break;
             }
             case Js::OpCode::CommitScope:
+            case Js::OpCode::GetCachedFunc:
             {
-                return this->currentInstr->GetSrc1()->IsScopeObjOpnd(currFunc);
+                return !doScopeObjCreation && this->currentInstr->GetSrc1()->IsScopeObjOpnd(currFunc);
             }
             case Js::OpCode::BrFncCachedScopeEq:
             case Js::OpCode::BrFncCachedScopeNeq:
             {
-                return this->currentInstr->GetSrc2()->IsScopeObjOpnd(currFunc);
+                return !doScopeObjCreation && this->currentInstr->GetSrc2()->IsScopeObjOpnd(currFunc);
+            }
+            case Js::OpCode::CallHelper:
+            {
+                if (!doScopeObjCreation && this->currentInstr->GetSrc1()->AsHelperCallOpnd()->m_fnHelper == IR::JnHelperMethod::HelperOP_InitCachedFuncs)
+                {
+                    IR::RegOpnd * scopeObjOpnd = this->currentInstr->GetSrc2()->GetStackSym()->GetInstrDef()->GetSrc1()->AsRegOpnd();
+                    return scopeObjOpnd->IsScopeObjOpnd(currFunc);
+                }
+                break;
             }
         }
     }
@@ -2933,7 +2948,7 @@ BackwardPass::CanDeadStoreInstrForScopeObjRemoval(Sym *sym) const
 * This is for Eliminating Scope Object Creation during Heap arguments optimization.
 */
 bool
-BackwardPass::DeadStoreOrChangeInstrForScopeObjRemoval()
+BackwardPass::DeadStoreOrChangeInstrForScopeObjRemoval(IR::Instr ** pInstrPrev)
 {
     IR::Instr * instr = this->currentInstr;
     Func * currFunc = instr->m_func;
@@ -2996,6 +3011,54 @@ BackwardPass::DeadStoreOrChangeInstrForScopeObjRemoval()
                 }
                 break;
             }
+            case Js::OpCode::CallHelper:
+            {
+                //Remove the CALL and all its Argout instrs.
+                if (instr->GetSrc1()->AsHelperCallOpnd()->m_fnHelper == IR::JnHelperMethod::HelperOP_InitCachedFuncs)
+                {
+                    IR::RegOpnd * scopeObjOpnd = instr->GetSrc2()->GetStackSym()->GetInstrDef()->GetSrc1()->AsRegOpnd();
+                    if (scopeObjOpnd->IsScopeObjOpnd(currFunc))
+                    {
+                        IR::Instr * instrDef = instr;
+                        IR::Instr * nextInstr = instr->m_next;
+
+                        while (instrDef != nullptr)
+                        {
+                            IR::Instr * instrToDelete = instrDef;
+                            if (instrDef->GetSrc2() != nullptr)
+                            {
+                                instrDef = instrDef->GetSrc2()->GetStackSym()->GetInstrDef();
+                                Assert(instrDef->m_opcode == Js::OpCode::ArgOut_A);
+                            }
+                            else
+                            {
+                                instrDef = nullptr;
+                            }
+                            instrToDelete->Remove();
+                        }
+                        Assert(nextInstr != nullptr);
+                        *pInstrPrev = nextInstr->m_prev;
+                        return true;
+                    }
+                }
+                break;
+            }
+            case Js::OpCode::GetCachedFunc:
+            {
+                // <dst> = GetCachedFunc <scopeObject>, <functionNum>
+                // is converted to 
+                // <dst> = NewScFunc <functionNum>, <env: FrameDisplay>
+
+                if (instr->GetSrc1()->IsScopeObjOpnd(currFunc))
+                {
+                    instr->m_opcode = Js::OpCode::NewScFunc;
+                    IR::Opnd * intConstOpnd = instr->UnlinkSrc2();
+
+                    instr->ReplaceSrc1(intConstOpnd);
+                    instr->SetSrc2(IR::RegOpnd::New(currFunc->GetLocalFrameDisplaySym(), IRType::TyVar, currFunc));
+                }
+                break;
+            }
         }
     }
     return false;
@@ -3005,7 +3068,7 @@ IR::Instr *
 BackwardPass::TryChangeInstrForStackArgOpt()
 {
     IR::Instr * instr = this->currentInstr;
-    if (instr->DoStackArgsOpt(this->func))
+    if (tag == Js::DeadStorePhase && instr->DoStackArgsOpt(this->func))
     {
         switch (instr->m_opcode)
         {
@@ -3035,6 +3098,22 @@ BackwardPass::TryChangeInstrForStackArgOpt()
             }
         }
     }
+
+    /*
+    *   Scope Object Sym is kept alive in all code paths.
+    *   -This is to facilitate Bailout to record the live Scope object Sym, whenever required.
+    *   -Reason for doing is this because - Scope object has to be implicitly live whenever Heap Arguments object is live.
+    *   -When we restore HeapArguments object in the bail out path, it expects the scope object also to be restored - if one was created.
+    *   -We do not know detailed information about Heap arguments obj syms(aliasing etc.) until we complete Forward Pass. 
+    *   -And we want to avoid dead sym clean up (in this case, scope object though not explicitly live, it is live implicitly) during Block merging in the forward pass. 
+    *   -Hence this is the optimal spot to do this.
+    */
+
+    if (tag == Js::BackwardPhase && instr->m_func->GetScopeObjSym() != nullptr)
+    {
+        this->currentBlock->upwardExposedUses->Set(instr->m_func->GetScopeObjSym()->m_id);
+    }
+
     return nullptr;
 }
 
@@ -6610,13 +6689,8 @@ BackwardPass::DeadStoreInstr(IR::Instr *instr)
 #endif
         PropertySym *unusedPropertySym = nullptr;
 
-        // Do not track the Scope Obj - we will be restoring it in the bailout path while restoring Heap arguments object.
-        // See InterpreterStackFrame::TrySetFrameObjectInHeapArgObj
-        if (!(instr->m_func->IsStackArgsEnabled() && instr->m_opcode == Js::OpCode::LdSlotArr &&
-            instr->GetSrc1() && instr->GetSrc1()->IsScopeObjOpnd(instr->m_func)))
-        {
-            GlobOpt::TrackByteCodeSymUsed(instr, this->currentBlock->byteCodeUpwardExposedUsed, &unusedPropertySym);
-        }
+        GlobOpt::TrackByteCodeSymUsed(instr, this->currentBlock->byteCodeUpwardExposedUsed, &unusedPropertySym);
+        
 #if DBG
         BVSparse<JitArenaAllocator> tempBv2(this->tempAlloc);
         tempBv2.Copy(this->currentBlock->byteCodeUpwardExposedUsed);

lib/Backend/BackwardPass.h

@@ -30,7 +30,8 @@ class BackwardPass
     IR::Instr * TryChangeInstrForStackArgOpt();
     void InsertArgInsForFormals();
     void ProcessBailOnStackArgsOutOfActualsRange();
-    bool DeadStoreOrChangeInstrForScopeObjRemoval();
+    void MarkScopeObjSymUseForStackArgOpt();
+    bool DeadStoreOrChangeInstrForScopeObjRemoval(IR::Instr ** pInstrPrev);
     void ProcessUse(IR::Opnd * opnd);
     bool ProcessDef(IR::Opnd * opnd);
     void ProcessTransfers(IR::Instr * instr);

lib/Backend/BailOut.cpp

@@ -997,6 +997,12 @@ BailOutRecord::RestoreValue(IR::BailOutKind bailOutKind, Js::JavascriptCallStack
         }
     }
 
+    Js::RegSlot localClosureReg = newInstance->function->GetFunctionBody()->GetLocalClosureRegister();
+    if (regSlot == localClosureReg)
+    {
+        this->globalBailOutRecordTable->isScopeObjRestored = true;
+    }
+
     values[regSlot] = value;
 
     BAILOUT_VERBOSE_TRACE(newInstance->function->GetFunctionBody(), bailOutKind, _u("\n"));
@@ -1651,9 +1657,13 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
 
     if (bailOutRecord->globalBailOutRecordTable->hasStackArgOpt)
     {
-        newInstance->TrySetFrameObjectInHeapArgObj(functionScriptContext, bailOutRecord->globalBailOutRecordTable->hasNonSimpleParams);
+        newInstance->TrySetFrameObjectInHeapArgObj(functionScriptContext, bailOutRecord->globalBailOutRecordTable->hasNonSimpleParams, 
+            bailOutRecord->globalBailOutRecordTable->isScopeObjRestored);
     }
-    
+
+    //Reset the value for tracking the restoration during next bail out.
+    bailOutRecord->globalBailOutRecordTable->isScopeObjRestored = false;
+
     uint32 innerScopeCount = executeFunction->GetInnerScopeCount();
     for (uint32 i = 0; i < innerScopeCount; i++)
     {

lib/Backend/BailOut.h

@@ -471,6 +471,7 @@ struct GlobalBailOutRecordDataTable
     bool isLoopBody;
     bool hasNonSimpleParams;
     bool hasStackArgOpt;
+    bool isScopeObjRestored;
     void Finalize(NativeCodeData::Allocator *allocator, JitArenaAllocator *tempAlloc);
     void AddOrUpdateRow(JitArenaAllocator *allocator, uint32 bailOutRecordId, uint32 regSlot, bool isFloat, bool isInt,
         bool isSimd128F4, bool isSimd128I4, bool isSimd128I8, bool isSimd128I16, bool isSimd128U4, bool isSimd128U8, bool isSimd128U16, bool isSimd128B4, bool isSimd128B8, bool isSimd128B16,

lib/Backend/FlowGraph.cpp

@@ -2590,7 +2590,18 @@ FlowGraph::RemoveInstr(IR::Instr *instr, GlobOpt * globOpt)
             return instr;
         }
 
+        /*
+        *   Scope object has to be implicitly live whenever Heap Arguments object is live.
+        *       - When we restore HeapArguments object in the bail out path, it expects the scope object also to be restored - if one was created.
+        */
         Js::OpCode opcode = instr->m_opcode;
+        if (opcode == Js::OpCode::LdElemI_A && instr->DoStackArgsOpt(this->func) &&
+            globOpt->IsArgumentsOpnd(instr->GetSrc1()) && instr->m_func->GetScopeObjSym())
+        {
+            IR::Instr * byteCodeUsesInstr = IR::ByteCodeUsesInstr::New(instr, instr->m_func->GetScopeObjSym()->m_id);
+            instr->InsertAfter(byteCodeUsesInstr);
+        }
+
         IR::ByteCodeUsesInstr * newByteCodeUseInstr = globOpt->ConvertToByteCodeUses(instr);
         if (newByteCodeUseInstr != nullptr)
         {

lib/Backend/GlobOpt.cpp

@@ -3944,7 +3944,7 @@ GlobOpt::TrackInstrsForScopeObjectRemoval(IR::Instr * instr)
 
     if (instr->m_opcode == Js::OpCode::Ld_A && src1->IsRegOpnd())
     {
-        AssertMsg(!src1->IsScopeObjOpnd(instr->m_func), "There can be no aliasing for scope object.");
+        AssertMsg(!instr->m_func->IsStackArgsEnabled() || !src1->IsScopeObjOpnd(instr->m_func), "There can be no aliasing for scope object.");
     }
 
     // The following is to track formals array for Stack Arguments optimization with Formals
@@ -4063,7 +4063,7 @@ GlobOpt::OptArguments(IR::Instr *instr)
                 StackSym * scopeObjSym = instr->GetSrc1()->GetStackSym();
                 Assert(scopeObjSym);
                 Assert(scopeObjSym->GetInstrDef()->m_opcode == Js::OpCode::InitCachedScope || scopeObjSym->GetInstrDef()->m_opcode == Js::OpCode::NewScopeObject);
-                instr->m_func->SetScopeObjSym(scopeObjSym);
+                Assert(instr->m_func->GetScopeObjSym() == scopeObjSym);
 
                 if (PHASE_VERBOSE_TRACE1(Js::StackArgFormalsOptPhase))
                 {
@@ -4134,6 +4134,7 @@ GlobOpt::OptArguments(IR::Instr *instr)
         {
             instr->usesStackArgumentsObject = true;
         }
+
         break;
     }
     case Js::OpCode::LdLen_A:
@@ -4147,6 +4148,11 @@ GlobOpt::OptArguments(IR::Instr *instr)
     }
     case Js::OpCode::ArgOut_A_InlineBuiltIn:
     {
+        if (IsArgumentsOpnd(src1))
+        {
+            instr->usesStackArgumentsObject = true;
+        }
+
         if (IsArgumentsOpnd(src1) &&
             src1->AsRegOpnd()->m_sym->GetInstrDef()->m_opcode == Js::OpCode::BytecodeArgOutCapture)
         {
@@ -4180,7 +4186,14 @@ GlobOpt::OptArguments(IR::Instr *instr)
     case Js::OpCode::BailOnNotStackArgs:
     case Js::OpCode::ArgOut_A_FromStackArgs:
     case Js::OpCode::BytecodeArgOutUse:
+    {
+        if (src1 && IsArgumentsOpnd(src1))
+        {
+            instr->usesStackArgumentsObject = true;
+        }
+        
         break;
+    }
 
     default:
         {

lib/Backend/GlobOpt.h

@@ -1292,6 +1292,7 @@ class GlobOpt
 
     IR::ByteCodeUsesInstr * ConvertToByteCodeUses(IR::Instr * isntr);
     bool GetIsAsmJSFunc()const{ return isAsmJSFunc; };
+    BOOLEAN                 IsArgumentsOpnd(IR::Opnd* opnd);
 private:
     bool                    IsLoopPrePass() const { return this->prePassLoop != nullptr; }
     void                    OptBlock(BasicBlock *block);
@@ -1335,7 +1336,6 @@ class GlobOpt
     IR::Instr *             SetTypeCheckBailOut(IR::Opnd *opnd, IR::Instr *instr, BailOutInfo *bailOutInfo);
     void                    OptArguments(IR::Instr *Instr);
     void                    TrackInstrsForScopeObjectRemoval(IR::Instr * instr);
-    BOOLEAN                 IsArgumentsOpnd(IR::Opnd* opnd);
     bool                    AreFromSameBytecodeFunc(IR::RegOpnd* src1, IR::RegOpnd* dst);
     void                    TrackArgumentsSym(IR::RegOpnd* opnd);
     void                    ClearArgumentsSym(IR::RegOpnd* opnd);

lib/Backend/IRBuilder.cpp

@@ -1589,6 +1589,10 @@ IRBuilder::BuildReg1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot R0)
             Js::RegSlot regFrameObj = m_func->GetJITFunctionBody()->GetLocalClosureReg();
             Assert(regFrameObj != Js::Constants::NoRegister);
             srcOpnd = BuildSrcOpnd(regFrameObj);
+            if (m_func->GetJITFunctionBody()->GetInParamsCount() > 1)
+            {
+                m_func->SetScopeObjSym(srcOpnd->GetStackSym());
+            }
         }
         else
         {
@@ -1617,6 +1621,10 @@ IRBuilder::BuildReg1(Js::OpCode newOpcode, uint32 offset, Js::RegSlot R0)
             Js::Throw::FatalInternalError();
         }
         srcOpnd = BuildSrcOpnd(m_func->GetJITFunctionBody()->GetLocalClosureReg());
+        if (m_func->GetJITFunctionBody()->GetInParamsCount() > 1)
+        {
+            m_func->SetScopeObjSym(srcOpnd->GetStackSym());
+        }
         isNotInt = true;
         break;