Rework how captured values are written to GeneratorBailInInstrs

chakra-core · Sep 24, 2019 · a0cdafa · a0cdafa
1 parent 7e29961
commit a0cdafa
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 80 deletions.
diff --git a/lib/Backend/BackwardPass.cpp b/lib/Backend/BackwardPass.cpp
@@ -1770,9 +1770,17 @@ BackwardPass::ProcessBailOutConstants(BailOutInfo * bailOutInfo, BVSparse<JitAre
     }
     NEXT_SLISTBASE_ENTRY;
 
+    auto* bailInInstr = IR::GeneratorBailInInstr::FromBailOutInfo(bailOutInfo);
+
     // Find other constants that we need to restore
     FOREACH_SLISTBASE_ENTRY_EDITING(ConstantStackSymValue, value, &bailOutInfo->capturedValues->constantValues, iter)
     {
+        if (bailInInstr)
+        {
+            // Store all captured constant values for the corresponding bailin instr
+            bailInInstr->capturedValues.constantValues.PrependNode(this->func->m_alloc, value);
+        }
+
         if (byteCodeUpwardExposedUsed->TestAndClear(value.Key()->m_id) || bailoutReferencedArgSymsBv->TestAndClear(value.Key()->m_id))
         {
             // Constant need to be restore, move it to the restore list
@@ -1806,6 +1814,8 @@ BackwardPass::ProcessBailOutCopyProps(BailOutInfo * bailOutInfo, BVSparse<JitAre
     BasicBlock * block = this->currentBlock;
     BVSparse<JitArenaAllocator> * upwardExposedUses = block->upwardExposedUses;
 
+    auto* bailInInstr = IR::GeneratorBailInInstr::FromBailOutInfo(bailOutInfo);
+
     // Find other copy prop that we need to restore
     FOREACH_SLISTBASE_ENTRY_EDITING(CopyPropSyms, copyPropSyms, &bailOutInfo->capturedValues->copyPropSyms, iter)
     {
@@ -1814,6 +1824,13 @@ BackwardPass::ProcessBailOutCopyProps(BailOutInfo * bailOutInfo, BVSparse<JitAre
         Assert(!copyPropSyms.Value()->IsTypeSpec());
         if (byteCodeUpwardExposedUsed->TestAndClear(copyPropSyms.Key()->m_id) || bailoutReferencedArgSymsBv->TestAndClear(copyPropSyms.Key()->m_id))
         {
+            if (bailInInstr)
+            {
+                // Copy all copyprop syms into the corresponding bail-in instr so that
+                // we can map the correct symbols to restore during bail-in
+                bailInInstr->capturedValues.copyPropSyms.PrependNode(allocator, copyPropSyms);
+            }
+
             // This copy-prop sym needs to be restored; add it to the restore list.
 
             /*
@@ -2561,30 +2578,6 @@ BackwardPass::NeedBailOutOnImplicitCallsForTypedArrayStore(IR::Instr* instr)
     return false;
 }
 
-IR::Instr*
-BackwardPass::ProcessPendingPreOpBailOutInfoForYield(IR::Instr* const currentInstr)
-{
-    Assert(currentInstr->m_opcode == Js::OpCode::Yield);
-    IR::GeneratorBailInInstr* bailInInstr = currentInstr->m_next->m_next->AsGeneratorBailInInstr();
-
-    BailOutInfo* bailOutInfo = currentInstr->GetBailOutInfo();
-
-    // Make a copy of all detected constant values before we actually process
-    // the bailout info since we will then remove any values that don't need
-    // to be restored for the normal bailout cases. As for yields, we still
-    // need them for our bailin code.
-    bailInInstr->SetConstantValues(bailOutInfo->capturedValues->constantValues);
-
-    IR::Instr* ret = this->ProcessPendingPreOpBailOutInfo(currentInstr);
-
-    // We will need list of symbols that have been copy-prop'd to map the correct
-    // symbols to restore during bail-in. Since this list is cleared during
-    // FillBailOutRecord, make a copy of it now.
-    bailInInstr->SetCopyPropSyms(bailOutInfo->usedCapturedValues->copyPropSyms);
-
-    return ret;
-}
-
 IR::Instr*
 BackwardPass::ProcessPendingPreOpBailOutInfo(IR::Instr *const currentInstr)
 {
@@ -3887,17 +3880,10 @@ BackwardPass::ProcessBlock(BasicBlock * block)
         // bail-in
         if (instr->IsGeneratorBailInInstr() && this->currentBlock->upwardExposedUses)
         {
-            instr->AsGeneratorBailInInstr()->SetUpwardExposedUses(*this->currentBlock->upwardExposedUses);
+            instr->AsGeneratorBailInInstr()->upwardExposedUses.Copy(this->currentBlock->upwardExposedUses);
         }
 
-        if (instr->m_opcode == Js::OpCode::Yield)
-        {
-            instrPrev = ProcessPendingPreOpBailOutInfoForYield(instr);
-        }
-        else
-        {
-            instrPrev = ProcessPendingPreOpBailOutInfo(instr);
-        }
+        instrPrev = ProcessPendingPreOpBailOutInfo(instr);
 
 #if DBG_DUMP
         TraceInstrUses(block, instr, false);

diff --git a/lib/Backend/BackwardPass.h b/lib/Backend/BackwardPass.h
@@ -45,7 +45,6 @@ class BackwardPass
     bool ProcessByteCodeUsesInstr(IR::Instr * instr);
     bool ProcessBailOutInfo(IR::Instr * instr);
     void ProcessBailOutInfo(IR::Instr * instr, BailOutInfo * bailOutInfo);
-    IR::Instr* ProcessPendingPreOpBailOutInfoForYield(IR::Instr* const currentInstr);
     IR::Instr* ProcessPendingPreOpBailOutInfo(IR::Instr *const currentInstr);
     void ClearDstUseForPostOpLazyBailOut(IR::Instr *instr);
     void ProcessBailOutArgObj(BailOutInfo * bailOutInfo, BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed);

diff --git a/lib/Backend/IR.cpp b/lib/Backend/IR.cpp
@@ -4263,6 +4263,19 @@ GeneratorBailInInstr::New(IR::Instr* yieldInstr, Func* func)
     return labelInstr;
 }
 
+GeneratorBailInInstr* GeneratorBailInInstr::FromBailOutInfo(BailOutInfo* bailOutInfo)
+{
+    Assert(bailOutInfo);
+    IR::Instr* instr = bailOutInfo->bailOutInstr;
+    if (!instr || instr->m_opcode != Js::OpCode::Yield)
+    {
+        return nullptr;
+    }
+
+    Assert(instr->m_next && instr->m_next->m_next);
+    return instr->m_next->m_next->AsGeneratorBailInInstr();
+}
+
 #if ENABLE_DEBUG_CONFIG_OPTIONS
 ///----------------------------------------------------------------------------
 ///

diff --git a/lib/Backend/IR.h b/lib/Backend/IR.h
@@ -1117,53 +1117,21 @@ class PragmaInstr : public Instr
 class GeneratorBailInInstr : public LabelInstr
 {
 private:
-    GeneratorBailInInstr(JitArenaAllocator* allocator, IR::Instr* yieldInstr):
-        LabelInstr(allocator), allocator(allocator), yieldInstr(yieldInstr), upwardExposedUses(allocator)
+    GeneratorBailInInstr(JitArenaAllocator* allocator, IR::Instr* yieldInstr) :
+        LabelInstr(allocator),
+        yieldInstr(yieldInstr),
+        upwardExposedUses(allocator)
     {
         Assert(yieldInstr != nullptr && yieldInstr->m_opcode == Js::OpCode::Yield);
-        this->usedCapturedValues = JitAnew(allocator, CapturedValues);
     }
 
-    JitArenaAllocator* const allocator;
-    IR::Instr* const yieldInstr;
-    CapturedValues* usedCapturedValues;
+public:
+    IR::Instr* yieldInstr;
+    CapturedValues capturedValues;
     BVSparse<JitArenaAllocator> upwardExposedUses;
 
-public:
     static GeneratorBailInInstr* New(IR::Instr* yieldInstr, Func* func);
-
-    IR::Instr* GetYieldInstr() const
-    {
-        return this->yieldInstr;
-    }
-
-    const CapturedValues& GetCapturedValues() const
-    {
-        return *this->usedCapturedValues;
-    }
-
-    const BVSparse<JitArenaAllocator>& GetUpwardExposedUses() const
-    {
-        return this->upwardExposedUses;
-    }
-
-    void SetCopyPropSyms(const SListBase<CopyPropSyms>& copyPropSyms)
-    {
-        this->usedCapturedValues->copyPropSyms.Clear(this->allocator);
-        copyPropSyms.CopyTo(this->allocator , this->usedCapturedValues->copyPropSyms);
-    }
-
-    void SetConstantValues(const SListBase<ConstantStackSymValue>& constantValues)
-    {
-        this->usedCapturedValues->constantValues.Clear(this->allocator);
-        constantValues.CopyTo(this->allocator, this->usedCapturedValues->constantValues);
-    }
-
-    void SetUpwardExposedUses(const BVSparse<JitArenaAllocator>& other)
-    {
-        this->upwardExposedUses.ClearAll();
-        this->upwardExposedUses.Or(&other);
-    }
+    static GeneratorBailInInstr* FromBailOutInfo(BailOutInfo* bailOutInfo);
 };
 
 template <typename InstrType>

diff --git a/lib/Backend/LinearScan.cpp b/lib/Backend/LinearScan.cpp
@@ -5004,7 +5004,7 @@ void LinearScan::GeneratorBailIn::SpillRegsForBailIn()
 //
 IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr* bailInInstr)
 {
-    BailOutInfo* bailOutInfo = bailInInstr->GetYieldInstr()->GetBailOutInfo();
+    BailOutInfo* bailOutInfo = bailInInstr->yieldInstr->GetBailOutInfo();
 
     Assert(!bailOutInfo->capturedValues || bailOutInfo->capturedValues->constantValues.Empty());
     Assert(!bailOutInfo->capturedValues || bailOutInfo->capturedValues->copyPropSyms.Empty());
@@ -5045,14 +5045,14 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
 
     this->BuildBailInSymbolList(
         *bailOutInfo->byteCodeUpwardExposedUsed,
-        bailInInstr->GetUpwardExposedUses(),
-        bailInInstr->GetCapturedValues()
+        bailInInstr->upwardExposedUses,
+        bailInInstr->capturedValues
     );
 
     this->InsertRestoreSymbols(
         *bailOutInfo->byteCodeUpwardExposedUsed,
-        bailInInstr->GetUpwardExposedUses(),
-        bailInInstr->GetCapturedValues(),
+        bailInInstr->upwardExposedUses,
+        bailInInstr->capturedValues,
         insertionPoint
     );
     Assert(!this->func->IsStackArgsEnabled());

diff --git a/lib/Backend/Lower.cpp b/lib/Backend/Lower.cpp
@@ -29088,7 +29088,7 @@ void Lowerer::LowerGeneratorHelper::InsertNullOutGeneratorFrameInEpilogue(IR::La
 
     IR::IndirOpnd* indirOpnd = IR::IndirOpnd::New(dstOpnd, Js::JavascriptGenerator::GetFrameOffset(), TyMachPtr, this->func);
     IR::AddrOpnd* addrOpnd = IR::AddrOpnd::NewNull(this->func);
-    InsertMove(indirOpnd, addrOpnd, insertionPoint);
+    InsertMove(indirOpnd, addrOpnd, insertionPoint, false /* generateWriteBarrier */);
 }
 
 void

diff --git a/test/es6/generator-jit-bugs.js b/test/es6/generator-jit-bugs.js
@@ -0,0 +1,20 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// Test 1 - const that is unused/replaced with undefined
+function* foo() {
+    const temp2 = null;
+    while (true) {
+        yield temp2;
+    }
+}
+
+const gen = foo();
+
+gen.next();
+gen.next();
+gen.next();
+
+print("Pass");
diff --git a/test/es6/rlexe.xml b/test/es6/rlexe.xml
@@ -126,6 +126,13 @@
       <tags>exclude_dynapogo</tags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>generator-jit-bugs.js</files>
+      <compile-flags>-JitES6Generators</compile-flags>
+      <tags>exclude_nonative</tags>
+    </default>
+  </test>
   <test>
     <default>
       <files>proto_basic.js</files>