Further gen fixes

lib/Backend/Func.cpp

@@ -150,7 +150,6 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
     , m_forInEnumeratorArrayOffset(-1)
     , argInsCount(0)
     , m_globalObjTypeSpecFldInfoArray(nullptr)
-    , m_generatorFrameSym(nullptr)
 #if LOWER_SPLIT_INT64
     , m_int64SymPairMap(nullptr)
 #endif

lib/Backend/Func.h

@@ -1064,23 +1064,11 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
     StackSym* m_loopParamSym;
     StackSym* m_bailoutReturnValueSym;
     StackSym* m_hasBailedOutSym;
-    StackSym* m_generatorFrameSym;
 
 public:
     StackSym* tempSymDouble;
     StackSym* tempSymBool;
 
-    void SetGeneratorFrameSym(StackSym* sym)
-    {
-        Assert(this->m_generatorFrameSym == nullptr);
-        this->m_generatorFrameSym = sym;
-    }
-
-    StackSym* GetGeneratorFrameSym() const
-    {
-        return this->m_generatorFrameSym;
-    }
-
     // StackSyms' corresponding getters/setters
     void SetInlineeFrameStartSym(StackSym* sym)
     {

lib/Backend/IRBuilder.cpp

@@ -7954,21 +7954,35 @@ IRBuilder::GeneratorJumpTable::BuildJumpTable()
 
     this->m_irBuilder->AddInstr(functionBegin, this->m_irBuilder->m_functionStartOffset);
 
-    // Save this value for later use
-    this->m_generatorFrameOpnd = genFrameOpnd;
-    this->m_func->SetGeneratorFrameSym(genFrameOpnd->GetStackSym());
-
     return this->m_irBuilder->m_lastInstr;
 }
 
 IR::RegOpnd*
 IRBuilder::GeneratorJumpTable::BuildForInEnumeratorArrayOpnd(uint32 offset)
 {   
-    Assert(this->m_generatorFrameOpnd != nullptr);
+    // s1 = Ld_A prm1
+    StackSym* genParamSym = StackSym::NewParamSlotSym(1, this->m_func);
+    this->m_func->SetArgOffset(genParamSym, LowererMD::GetFormalParamOffset() * MachPtr);
+
+    IR::SymOpnd* genParamOpnd = IR::SymOpnd::New(genParamSym, TyMachPtr, this->m_func);
+    IR::RegOpnd* genRegOpnd = IR::RegOpnd::New(TyMachPtr, this->m_func);
+    IR::Instr* instr = IR::Instr::New(Js::OpCode::Ld_A, genRegOpnd, genParamOpnd, this->m_func);
+    this->m_irBuilder->AddInstr(instr, offset);
+
+    // s2 = Ld_A s1[offset of JavascriptGenerator::frame]
+    IR::RegOpnd* genFrameOpnd = IR::RegOpnd::New(TyMachPtr, this->m_func);
+    instr = IR::Instr::New(
+        Js::OpCode::Ld_A,
+        genFrameOpnd,
+        POINTER_OFFSET(genRegOpnd, Js::JavascriptGenerator::GetFrameOffset(), GeneratorFrame),
+        this->m_func
+    );
+    this->m_irBuilder->AddInstr(instr, offset);
 
+    // s3 = Ld_A s2[offset of ForInEnumerator]
     IR::RegOpnd* forInEnumeratorArrayOpnd = IR::RegOpnd::New(TyMachPtr, this->m_func);
-    IR::Instr* instr = IR::Instr::New(Js::OpCode::Ld_A, forInEnumeratorArrayOpnd,
-        POINTER_OFFSET(this->m_generatorFrameOpnd, Js::InterpreterStackFrame::GetOffsetOfForInEnumerators(), ForInEnumerators),
+    instr = IR::Instr::New(Js::OpCode::Ld_A, forInEnumeratorArrayOpnd,
+        POINTER_OFFSET(genFrameOpnd, Js::InterpreterStackFrame::GetOffsetOfForInEnumerators(), ForInEnumerators),
         this->m_func
     );
     this->m_irBuilder->AddInstr(instr, offset);

lib/Backend/IRBuilder.h

@@ -366,8 +366,6 @@ class IRBuilder
         Func* const m_func;
         IRBuilder* const m_irBuilder;
 
-        IR::RegOpnd* m_generatorFrameOpnd = nullptr;
-
     public:
         GeneratorJumpTable(Func* func, IRBuilder* irBuilder);
         IR::Instr* BuildJumpTable();

lib/Backend/LinearScan.cpp

@@ -3339,7 +3339,7 @@ LinearScan::KillImplicitRegs(IR::Instr *instr)
         return;
     }
 
-    if (instr->m_opcode == Js::OpCode::Yield)
+    if (instr->m_opcode == Js::OpCode::GeneratorBailInLabel)
     {
         this->bailIn.SpillRegsForBailIn();
         return;
@@ -5041,6 +5041,7 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
     // - We don't need to restore argObjSyms because StackArgs is currently not enabled
     //   Commented out here in case we do want to enable it in the future:
     // this->InsertRestoreSymbols(bailOutInfo->capturedValues->argObjSyms, insertionPoint, saveInitializedReg);
+    Assert(!this->func->IsStackArgsEnabled());
     // 
     // - We move all argout symbols right before the call so we don't need to restore argouts either
 
@@ -5050,13 +5051,7 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
         bailInInstr->capturedValues
     );
 
-    this->InsertRestoreSymbols(
-        *bailOutInfo->byteCodeUpwardExposedUsed,
-        bailInInstr->upwardExposedUses,
-        bailInInstr->capturedValues,
-        insertionPoint
-    );
-    Assert(!this->func->IsStackArgsEnabled());
+    this->InsertRestoreSymbols(insertionPoint);
 
 #ifdef ENABLE_DEBUG_CONFIG_OPTIONS
     if (PHASE_TRACE(Js::Phase::BailInPhase, this->func))
@@ -5185,8 +5180,6 @@ void LinearScan::GeneratorBailIn::BuildBailInSymbolList(
         Assert(stackSym);
         Lifetime* lifetime = stackSym->scratch.linearScan.lifetime;
         if (
-            // Special backend symbols that don't need to be restored
-            (!stackSym->HasByteCodeRegSlot() && !this->NeedsReloadingBackendSymWhenBailingIn(stackSym)) ||
             // Symbols already in the constant table don't need to be restored either
             stackSym->IsFromByteCodeConstantTable() ||
             // Symbols having no lifetimes
@@ -5201,12 +5194,7 @@ void LinearScan::GeneratorBailIn::BuildBailInSymbolList(
     AssertOrFailFastMsg(unrestorableSymbols.IsEmpty(), "There are unrestorable backend-only symbols across yield points");
 }
 
-void LinearScan::GeneratorBailIn::InsertRestoreSymbols(
-    const BVSparse<JitArenaAllocator>& byteCodeUpwardExposedUses,
-    const BVSparse<JitArenaAllocator>& upwardExposedUses,
-    const CapturedValues& capturedValues,
-    BailInInsertionPoint& insertionPoint
-)
+void LinearScan::GeneratorBailIn::InsertRestoreSymbols(BailInInsertionPoint& insertionPoint)
 {
     FOREACH_SLISTBASE_ENTRY(BailInSymbol, bailInSymbol, this->bailInSymbols)
     {
@@ -5298,18 +5286,6 @@ void LinearScan::GeneratorBailIn::InsertRestoreSymbols(
     NEXT_SLISTBASE_ENTRY;
 }
 
-bool LinearScan::GeneratorBailIn::NeedsReloadingBackendSymWhenBailingIn(StackSym* sym) const
-{
-    // for-in enumerator in generator is loaded as part of the resume jump table.
-    // By the same reasoning as `initializedRegs`'s, we don't have to restore this whether or not it's been spilled.
-    if (this->func->GetGeneratorFrameSym() && this->func->GetGeneratorFrameSym()->m_id == sym->m_id)
-    {
-        return false;
-    }
-
-    return true;
-}
-
 bool LinearScan::GeneratorBailIn::NeedsReloadingSymWhenBailingIn(StackSym* sym) const
 {
     if (sym->IsFromByteCodeConstantTable())
@@ -5325,7 +5301,7 @@ bool LinearScan::GeneratorBailIn::NeedsReloadingSymWhenBailingIn(StackSym* sym)
 
     if (!sym->HasByteCodeRegSlot())
     {
-        return this->NeedsReloadingBackendSymWhenBailingIn(sym);
+        return true;
     }
 
     if (sym->IsConst())

lib/Backend/LinearScan.h

@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft. All rights reserved.
+// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #pragma once
@@ -290,18 +291,12 @@ class LinearScan
         IR::RegOpnd* const interpreterFrameRegOpnd;
         IR::RegOpnd* const tempRegOpnd;
 
-        bool NeedsReloadingBackendSymWhenBailingIn(StackSym* sym) const;
         bool NeedsReloadingSymWhenBailingIn(StackSym* sym) const;
         uint32 GetOffsetFromInterpreterStackFrame(Js::RegSlot regSlot) const;
         IR::SymOpnd* CreateGeneratorObjectOpnd() const;
 
         // Insert instructions to restore symbols in the `bailInSymbols` list
-        void InsertRestoreSymbols(
-            const BVSparse<JitArenaAllocator>& bytecodeUpwardExposedUses,
-            const BVSparse<JitArenaAllocator>& upwardExposedUses,
-            const CapturedValues& capturedValues,
-            BailInInsertionPoint& insertionPoint
-        );
+        void InsertRestoreSymbols(BailInInsertionPoint& insertionPoint);
 
         // Fill `bailInSymbols` list with all of the symbols that need to be restored
         void BuildBailInSymbolList(

lib/Backend/SccLiveness.cpp

@@ -260,9 +260,19 @@ SCCLiveness::Build()
             {
                 this->EndOpHelper(labelInstr);
             }
-            if (labelInstr->isOpHelper && !PHASE_OFF(Js::OpHelperRegOptPhase, this->func))
+            if (labelInstr->isOpHelper)
             {
-                this->lastOpHelperLabel = labelInstr;
+                if (!PHASE_OFF(Js::OpHelperRegOptPhase, this->func) &&
+                    !this->func->GetTopFunc()->GetJITFunctionBody()->IsCoroutine())
+                {
+                    this->lastOpHelperLabel = labelInstr;
+                }
+#ifdef DBG
+                else
+                {
+                    labelInstr->AsLabelInstr()->m_noHelperAssert = true;
+                }
+#endif
             }
         }
         else if (instr->IsBranchInstr() && !instr->AsBranchInstr()->IsMultiBranch())

test/es6/generator-jit-bugs.js

@@ -152,5 +152,21 @@ check(gf8().next().value.v8, 1.1);
 check(gf8().next().value.v8, 1.1);
 check(gf8().next().value.v8, 1.1);
 
+// Test 9 - Invalid OpHelperBlockReg spill
+title("Inner function access after for...in loop containing yield that is not hit")
+{ // Note this bug only occurred when generator was declared inside an outer scope block
+    let i = 0;
+    function* gf9() {
+        function test() {}
+        for (var unused in []) {
+            yield undefined;
+        }
+        if(++i < 4)
+            gf9().next()
+        test + 1;
+    }
+}
+check(gf9().next().done, true);
+
 
 print("pass");