Address comments

Author: nhat-nguyen

lib/Backend/BackwardPass.cpp

@@ -2489,7 +2489,7 @@ BackwardPass::UpdateImplicitCallBailOutKind(IR::Instr *const instr, bool needsBa
         // We decided that BailOutOnImplicitCall is needed. So lazy bailout is unnecessary
         // because we are already protected from potential side effects unless the operation
         // itself can change fields' values (StFld/StElem).
-        if (needsLazyBailOut && !instr->CanChangeValueWithoutImplicitCall())
+        if (needsLazyBailOut && !instr->CanChangeFieldValueWithoutImplicitCall())
         {
             instr->ClearLazyBailOut();
         }

lib/Backend/BailOut.cpp

@@ -87,7 +87,10 @@ void BailOutInfo::PartialDeepCopyTo(BailOutInfo * const other) const
 void
 BailOutInfo::Clear(JitArenaAllocator * allocator)
 {
-    // Currently, we don't have a case where we delete bailout info after we allocated the bailout record
+    // Previously, we don't have a case where we delete bailout info after we allocated the bailout record.
+    // However, since lazy bailouts can now be attached on helper call instructions, and those instructions
+    // might sometimes be removed in Peeps, we will hit those cases. Make sure that in such cases, lazy bailout
+    // is the only bailout reason we have.
     Assert(bailOutRecord == nullptr || BailOutInfo::OnlyHasLazyBailOut(bailOutRecord->bailOutKind));
     if (this->capturedValues && this->capturedValues->DecrementRefCount() == 0)
     {

lib/Backend/Encoder.cpp

@@ -1691,11 +1691,11 @@ Encoder::SaveLazyBailOutJitTransferData()
     {
         Assert(this->m_sortedLazyBailoutRecordList->Count() > 0);
         Assert(this->m_lazyBailOutThunkOffset != 0);
-        Assert(this->m_func->GetLazyBailOutRecordArgSlot() != nullptr);
+        Assert(this->m_func->GetLazyBailOutRecordSlot() != nullptr);
 
         auto nativeEntryPointData = this->m_func->GetInProcJITEntryPointInfo()->GetInProcNativeEntryPointData();
         nativeEntryPointData->SetSortedLazyBailOutRecordList(this->m_sortedLazyBailoutRecordList);
-        nativeEntryPointData->SetLazyBailOutRecordArgSlotOffset(this->m_func->GetLazyBailOutRecordArgSlot()->m_offset);
+        nativeEntryPointData->SetLazyBailOutRecordSlotOffset(this->m_func->GetLazyBailOutRecordSlot()->m_offset);
         nativeEntryPointData->SetLazyBailOutThunkOffset(this->m_lazyBailOutThunkOffset);
     }
 

lib/Backend/Func.cpp

@@ -155,9 +155,8 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
 #ifdef RECYCLER_WRITE_BARRIER_JIT
     , m_lowerer(nullptr)
 #endif
-    , m_lazyBailOutRecordArgSlot(nullptr)
+    , m_lazyBailOutRecordSlot(nullptr)
     , hasLazyBailOut(false)
-    , lazyBailOutThunkLabel(nullptr)
 {
 
     Assert(this->IsInlined() == !!runtimeInfo);
@@ -2075,24 +2074,6 @@ Func::GetForInEnumeratorArrayOffset() const
         + this->m_forInLoopBaseDepth * sizeof(Js::ForInObjectEnumerator);
 }
 
-void
-Func::SetLazyBailOutThunkLabel(IR::LabelInstr *label)
-{
-    Assert(label != nullptr &&
-        label->m_opcode == Js::OpCode::LazyBailOutThunkLabel &&
-        this->lazyBailOutThunkLabel == nullptr &&
-        this->IsInPhase(Js::Phase::FinalLowerPhase));
-
-    this->lazyBailOutThunkLabel = label;
-}
-
-IR::LabelInstr *
-Func::GetLazyBailOutThunkLabel() const
-{
-    Assert(this->lazyBailOutThunkLabel != nullptr);
-    return this->lazyBailOutThunkLabel;
-}
-
 void
 Func::SetHasLazyBailOut()
 {
@@ -2110,27 +2091,27 @@ Func::HasLazyBailOut() const
 }
 
 void
-Func::AllocateLazyBailOutRecordArgSlotIfNeeded()
+Func::EnsureLazyBailOutRecordSlot()
 {
-    if (this->m_lazyBailOutRecordArgSlot == nullptr)
+    if (this->m_lazyBailOutRecordSlot == nullptr)
     {
-        this->m_lazyBailOutRecordArgSlot = StackSym::New(TyMachPtr, this);
-        this->StackAllocate(this->m_lazyBailOutRecordArgSlot, MachPtr);
+        this->m_lazyBailOutRecordSlot = StackSym::New(TyMachPtr, this);
+        this->StackAllocate(this->m_lazyBailOutRecordSlot, MachPtr);
     }
 }
 
 StackSym *
-Func::GetLazyBailOutRecordArgSlot() const
+Func::GetLazyBailOutRecordSlot() const
 {
-    Assert(this->m_lazyBailOutRecordArgSlot != nullptr);
-    return this->m_lazyBailOutRecordArgSlot;
+    Assert(this->m_lazyBailOutRecordSlot != nullptr);
+    return this->m_lazyBailOutRecordSlot;
 }
 
 bool
 Func::ShouldDoLazyBailOut() const
 {
 #if defined(_M_X64)
-    if (PHASE_OFF1(Js::LazyBailoutPhase) ||
+    if (PHASE_ON1(Js::LazyBailoutPhase) ||
         this->GetJITFunctionBody()->IsAsmJsMode() || // don't have bailouts in asm.js
         this->HasTry() ||                            // lazy bailout in function with try/catch not supported for now
                                                      // `EHBailoutPatchUp` set a `hasBailedOut` bit to rethrow the exception in the interpreter

lib/Backend/Func.h

@@ -1103,17 +1103,13 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
     // Lazy bailout
 private:
     bool                hasLazyBailOut : 1;
-    IR::LabelInstr*     lazyBailOutThunkLabel;
-    StackSym *          m_lazyBailOutRecordArgSlot;
+    StackSym *          m_lazyBailOutRecordSlot;
 
 public:
-    void AllocateLazyBailOutRecordArgSlotIfNeeded();
-    StackSym *GetLazyBailOutRecordArgSlot() const;
+    void EnsureLazyBailOutRecordSlot();
+    StackSym *GetLazyBailOutRecordSlot() const;
     void SetHasLazyBailOut();
     bool HasLazyBailOut() const;
-
-    void SetLazyBailOutThunkLabel(IR::LabelInstr *label);
-    IR::LabelInstr *GetLazyBailOutThunkLabel() const;
     bool ShouldDoLazyBailOut() const;
 };
 

lib/Backend/GlobOpt.cpp

@@ -2682,12 +2682,10 @@ GlobOpt::OptInstr(IR::Instr *&instr, bool* isInstrRemoved)
         }
     }
 
-#if defined(_M_X64)
     if (this->IsLazyBailOutCurrentlyNeeded(instr, src1Val, src2Val, isHoisted))
     {
         this->GenerateLazyBailOut(instr);
     }
-#endif
 
     if (CurrentBlockData()->capturedValuesCandidate && !this->IsLoopPrePass())
     {

lib/Backend/GlobOptBailOut.cpp

@@ -1214,10 +1214,12 @@ GlobOpt::MaySrcNeedBailOnImplicitCall(IR::Opnd const * opnd, Value const * val)
 bool
 GlobOpt::IsLazyBailOutCurrentlyNeeded(IR::Instr * instr, Value const * src1Val, Value const * src2Val, bool isHoisted) const
 {
+#ifdef _M_X64
+
     if (!this->func->ShouldDoLazyBailOut() ||
         this->IsLoopPrePass() ||
-        isHoisted ||
-        this->func->HasTry())
+        isHoisted
+    )
     {
         return false;
     }
@@ -1246,7 +1248,7 @@ GlobOpt::IsLazyBailOutCurrentlyNeeded(IR::Instr * instr, Value const * src1Val,
     }
 
     // We decided to do StackArgs optimization, which means that this instruction
-    // could only either be LoadElement or LoadTypeOf, and that it does not have
+    // could only either be LdElemI_A or TypeofElem, and that it does not have
     // an implicit call. So no need for lazy bailout.
     if (instr->HasBailOutInfo() && instr->GetBailOutKind() == IR::BailOnStackArgsOutOfActualsRange)
     {
@@ -1267,6 +1269,12 @@ GlobOpt::IsLazyBailOutCurrentlyNeeded(IR::Instr * instr, Value const * src1Val,
     // need lazy bailout of we think there might be implicit calls
     // or if there aren't any bailouts that prevent them from happening.
     return this->MayNeedBailOnImplicitCall(instr, src1Val, src2Val);
+
+#else // _M_X64
+
+    return false;
+
+#endif
 }
 
 void

lib/Backend/IR.cpp

@@ -1046,7 +1046,7 @@ bool IR::Instr::IsStElemVariant() const
         this->m_opcode == Js::OpCode::StElemC;
 }
 
-bool IR::Instr::CanChangeValueWithoutImplicitCall() const
+bool IR::Instr::CanChangeFieldValueWithoutImplicitCall() const
 {
     // TODO: Why is InitFld necessary?
     return this->IsStFldVariant() || this->IsStElemVariant();

lib/Backend/IR.h

@@ -75,7 +75,6 @@ struct CapturedValues
         }
 
         // Ignore refCount because other objects might still reference it
-        other->refCount;
     }
 };
 
@@ -341,7 +340,7 @@ class Instr
     bool            AreAllOpndsTypeSpecialized() const;
     bool            IsStFldVariant() const;
     bool            IsStElemVariant() const;
-    bool            CanChangeValueWithoutImplicitCall() const;
+    bool            CanChangeFieldValueWithoutImplicitCall() const;
     void            ClearLazyBailOut();
     bool            OnlyHasLazyBailOut() const;
     bool            HasLazyBailOut() const;

lib/Backend/LinearScan.cpp

@@ -217,18 +217,21 @@ LinearScan::RegAlloc()
         // missed allocating bailout records for them. Additionally, some instructions might
         // end up being lowered differently, so the lazy bailout is not on a CALL instruction
         // anymore. Use this opportunity to detect them.
-        // Note that this needs to be run with -ForcePostLowerGlobOptInstrString
+        // Note that the dump for the instruction will also be printed with -ForcePostLowerGlobOptInstrString
         if (instr->HasBailOutInfo() && instr->GetBailOutInfo()->bailOutRecord == nullptr)
         {
-            // The instruction has already been lowered, find the start to get the globopt dump
-            IR::Instr *curr = instr;
-            while (curr->globOptInstrString == nullptr)
+            if (CONFIG_FLAG(ForcePostLowerGlobOptInstrString))
             {
-                curr = curr->m_prev;
-            }
+                // The instruction has already been lowered, find the start to get the globopt dump
+                IR::Instr *curr = instr;
+                while (curr->globOptInstrString == nullptr)
+                {
+                    curr = curr->m_prev;
+                }
 
-            instr->Dump();
-            curr->DumpGlobOptInstrString();
+                instr->Dump();
+                curr->DumpGlobOptInstrString();
+            }
 
             AssertMsg(false, "Lazy bailout: bailOutRecord not allocated");
         }
@@ -4873,7 +4876,7 @@ LinearScan::ProcessLazyBailOut(IR::Instr *instr)
         // No lazy bailout for function with try/catch for now
         Assert(!this->func->HasTry());
 
-        this->func->AllocateLazyBailOutRecordArgSlotIfNeeded();
+        this->func->EnsureLazyBailOutRecordSlot();
 
         if (instr->GetBailOutInfo()->NeedsToRestoreUseOfDst())
         {