Skip to content

Commit 39f1ccd

Browse files
nhat-nguyennhat-nguyen
committed
Address comments
1 parent 981f260 commit 39f1ccd

19 files changed

+73
-83
lines changed

‎lib/Backend/BackwardPass.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2489,7 +2489,7 @@ BackwardPass::UpdateImplicitCallBailOutKind(IR::Instr *const instr, bool needsBa
24892489
// We decided that BailOutOnImplicitCall is needed. So lazy bailout is unnecessary
24902490
// because we are already protected from potential side effects unless the operation
24912491
// itself can change fields' values (StFld/StElem).
2492-
if (needsLazyBailOut && !instr->CanChangeValueWithoutImplicitCall())
2492+
if (needsLazyBailOut && !instr->CanChangeFieldValueWithoutImplicitCall())
24932493
{
24942494
instr->ClearLazyBailOut();
24952495
}

‎lib/Backend/BailOut.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,10 @@ void BailOutInfo::PartialDeepCopyTo(BailOutInfo * const other) const
8787
void
8888
BailOutInfo::Clear(JitArenaAllocator * allocator)
8989
{
90-
// Currently, we don't have a case where we delete bailout info after we allocated the bailout record
90+
// Previously, we don't have a case where we delete bailout info after we allocated the bailout record.
91+
// However, since lazy bailouts can now be attached on helper call instructions, and those instructions
92+
// might sometimes be removed in Peeps, we will hit those cases. Make sure that in such cases, lazy bailout
93+
// is the only bailout reason we have.
9194
Assert(bailOutRecord == nullptr || BailOutInfo::OnlyHasLazyBailOut(bailOutRecord->bailOutKind));
9295
if (this->capturedValues && this->capturedValues->DecrementRefCount() == 0)
9396
{

‎lib/Backend/Encoder.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1691,11 +1691,11 @@ Encoder::SaveLazyBailOutJitTransferData()
16911691
{
16921692
Assert(this->m_sortedLazyBailoutRecordList->Count() > 0);
16931693
Assert(this->m_lazyBailOutThunkOffset != 0);
1694-
Assert(this->m_func->GetLazyBailOutRecordArgSlot() != nullptr);
1694+
Assert(this->m_func->GetLazyBailOutRecordSlot() != nullptr);
16951695

16961696
auto nativeEntryPointData = this->m_func->GetInProcJITEntryPointInfo()->GetInProcNativeEntryPointData();
16971697
nativeEntryPointData->SetSortedLazyBailOutRecordList(this->m_sortedLazyBailoutRecordList);
1698-
nativeEntryPointData->SetLazyBailOutRecordArgSlotOffset(this->m_func->GetLazyBailOutRecordArgSlot()->m_offset);
1698+
nativeEntryPointData->SetLazyBailOutRecordSlotOffset(this->m_func->GetLazyBailOutRecordSlot()->m_offset);
16991699
nativeEntryPointData->SetLazyBailOutThunkOffset(this->m_lazyBailOutThunkOffset);
17001700
}
17011701

‎lib/Backend/Func.cpp

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -155,9 +155,8 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
155155
#ifdef RECYCLER_WRITE_BARRIER_JIT
156156
, m_lowerer(nullptr)
157157
#endif
158-
, m_lazyBailOutRecordArgSlot(nullptr)
158+
, m_lazyBailOutRecordSlot(nullptr)
159159
, hasLazyBailOut(false)
160-
, lazyBailOutThunkLabel(nullptr)
161160
{
162161

163162
Assert(this->IsInlined() == !!runtimeInfo);
@@ -2075,24 +2074,6 @@ Func::GetForInEnumeratorArrayOffset() const
20752074
+ this->m_forInLoopBaseDepth * sizeof(Js::ForInObjectEnumerator);
20762075
}
20772076

2078-
void
2079-
Func::SetLazyBailOutThunkLabel(IR::LabelInstr *label)
2080-
{
2081-
Assert(label != nullptr &&
2082-
label->m_opcode == Js::OpCode::LazyBailOutThunkLabel &&
2083-
this->lazyBailOutThunkLabel == nullptr &&
2084-
this->IsInPhase(Js::Phase::FinalLowerPhase));
2085-
2086-
this->lazyBailOutThunkLabel = label;
2087-
}
2088-
2089-
IR::LabelInstr *
2090-
Func::GetLazyBailOutThunkLabel() const
2091-
{
2092-
Assert(this->lazyBailOutThunkLabel != nullptr);
2093-
return this->lazyBailOutThunkLabel;
2094-
}
2095-
20962077
void
20972078
Func::SetHasLazyBailOut()
20982079
{
@@ -2110,27 +2091,27 @@ Func::HasLazyBailOut() const
21102091
}
21112092

21122093
void
2113-
Func::AllocateLazyBailOutRecordArgSlotIfNeeded()
2094+
Func::EnsureLazyBailOutRecordSlot()
21142095
{
2115-
if (this->m_lazyBailOutRecordArgSlot == nullptr)
2096+
if (this->m_lazyBailOutRecordSlot == nullptr)
21162097
{
2117-
this->m_lazyBailOutRecordArgSlot = StackSym::New(TyMachPtr, this);
2118-
this->StackAllocate(this->m_lazyBailOutRecordArgSlot, MachPtr);
2098+
this->m_lazyBailOutRecordSlot = StackSym::New(TyMachPtr, this);
2099+
this->StackAllocate(this->m_lazyBailOutRecordSlot, MachPtr);
21192100
}
21202101
}
21212102

21222103
StackSym *
2123-
Func::GetLazyBailOutRecordArgSlot() const
2104+
Func::GetLazyBailOutRecordSlot() const
21242105
{
2125-
Assert(this->m_lazyBailOutRecordArgSlot != nullptr);
2126-
return this->m_lazyBailOutRecordArgSlot;
2106+
Assert(this->m_lazyBailOutRecordSlot != nullptr);
2107+
return this->m_lazyBailOutRecordSlot;
21272108
}
21282109

21292110
bool
21302111
Func::ShouldDoLazyBailOut() const
21312112
{
21322113
#if defined(_M_X64)
2133-
if (PHASE_OFF1(Js::LazyBailoutPhase) ||
2114+
if (PHASE_ON1(Js::LazyBailoutPhase) ||
21342115
this->GetJITFunctionBody()->IsAsmJsMode() || // don't have bailouts in asm.js
21352116
this->HasTry() || // lazy bailout in function with try/catch not supported for now
21362117
// `EHBailoutPatchUp` set a `hasBailedOut` bit to rethrow the exception in the interpreter

‎lib/Backend/Func.h

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1103,17 +1103,13 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
11031103
// Lazy bailout
11041104
private:
11051105
bool hasLazyBailOut : 1;
1106-
IR::LabelInstr* lazyBailOutThunkLabel;
1107-
StackSym * m_lazyBailOutRecordArgSlot;
1106+
StackSym * m_lazyBailOutRecordSlot;
11081107

11091108
public:
1110-
void AllocateLazyBailOutRecordArgSlotIfNeeded();
1111-
StackSym *GetLazyBailOutRecordArgSlot() const;
1109+
void EnsureLazyBailOutRecordSlot();
1110+
StackSym *GetLazyBailOutRecordSlot() const;
11121111
void SetHasLazyBailOut();
11131112
bool HasLazyBailOut() const;
1114-
1115-
void SetLazyBailOutThunkLabel(IR::LabelInstr *label);
1116-
IR::LabelInstr *GetLazyBailOutThunkLabel() const;
11171113
bool ShouldDoLazyBailOut() const;
11181114
};
11191115

‎lib/Backend/GlobOpt.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2682,12 +2682,10 @@ GlobOpt::OptInstr(IR::Instr *&instr, bool* isInstrRemoved)
26822682
}
26832683
}
26842684

2685-
#if defined(_M_X64)
26862685
if (this->IsLazyBailOutCurrentlyNeeded(instr, src1Val, src2Val, isHoisted))
26872686
{
26882687
this->GenerateLazyBailOut(instr);
26892688
}
2690-
#endif
26912689

26922690
if (CurrentBlockData()->capturedValuesCandidate && !this->IsLoopPrePass())
26932691
{

‎lib/Backend/GlobOptBailOut.cpp

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1214,10 +1214,12 @@ GlobOpt::MaySrcNeedBailOnImplicitCall(IR::Opnd const * opnd, Value const * val)
12141214
bool
12151215
GlobOpt::IsLazyBailOutCurrentlyNeeded(IR::Instr * instr, Value const * src1Val, Value const * src2Val, bool isHoisted) const
12161216
{
1217+
#ifdef _M_X64
1218+
12171219
if (!this->func->ShouldDoLazyBailOut() ||
12181220
this->IsLoopPrePass() ||
1219-
isHoisted ||
1220-
this->func->HasTry())
1221+
isHoisted
1222+
)
12211223
{
12221224
return false;
12231225
}
@@ -1246,7 +1248,7 @@ GlobOpt::IsLazyBailOutCurrentlyNeeded(IR::Instr * instr, Value const * src1Val,
12461248
}
12471249

12481250
// We decided to do StackArgs optimization, which means that this instruction
1249-
// could only either be LoadElement or LoadTypeOf, and that it does not have
1251+
// could only either be LdElemI_A or TypeofElem, and that it does not have
12501252
// an implicit call. So no need for lazy bailout.
12511253
if (instr->HasBailOutInfo() && instr->GetBailOutKind() == IR::BailOnStackArgsOutOfActualsRange)
12521254
{
@@ -1267,6 +1269,12 @@ GlobOpt::IsLazyBailOutCurrentlyNeeded(IR::Instr * instr, Value const * src1Val,
12671269
// need lazy bailout of we think there might be implicit calls
12681270
// or if there aren't any bailouts that prevent them from happening.
12691271
return this->MayNeedBailOnImplicitCall(instr, src1Val, src2Val);
1272+
1273+
#else // _M_X64
1274+
1275+
return false;
1276+
1277+
#endif
12701278
}
12711279

12721280
void

‎lib/Backend/IR.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1046,7 +1046,7 @@ bool IR::Instr::IsStElemVariant() const
10461046
this->m_opcode == Js::OpCode::StElemC;
10471047
}
10481048

1049-
bool IR::Instr::CanChangeValueWithoutImplicitCall() const
1049+
bool IR::Instr::CanChangeFieldValueWithoutImplicitCall() const
10501050
{
10511051
// TODO: Why is InitFld necessary?
10521052
return this->IsStFldVariant() || this->IsStElemVariant();

‎lib/Backend/IR.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ struct CapturedValues
7575
}
7676

7777
// Ignore refCount because other objects might still reference it
78-
other->refCount;
7978
}
8079
};
8180

@@ -341,7 +340,7 @@ class Instr
341340
bool AreAllOpndsTypeSpecialized() const;
342341
bool IsStFldVariant() const;
343342
bool IsStElemVariant() const;
344-
bool CanChangeValueWithoutImplicitCall() const;
343+
bool CanChangeFieldValueWithoutImplicitCall() const;
345344
void ClearLazyBailOut();
346345
bool OnlyHasLazyBailOut() const;
347346
bool HasLazyBailOut() const;

‎lib/Backend/LinearScan.cpp

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -217,18 +217,21 @@ LinearScan::RegAlloc()
217217
// missed allocating bailout records for them. Additionally, some instructions might
218218
// end up being lowered differently, so the lazy bailout is not on a CALL instruction
219219
// anymore. Use this opportunity to detect them.
220-
// Note that this needs to be run with -ForcePostLowerGlobOptInstrString
220+
// Note that the dump for the instruction will also be printed with -ForcePostLowerGlobOptInstrString
221221
if (instr->HasBailOutInfo() && instr->GetBailOutInfo()->bailOutRecord == nullptr)
222222
{
223-
// The instruction has already been lowered, find the start to get the globopt dump
224-
IR::Instr *curr = instr;
225-
while (curr->globOptInstrString == nullptr)
223+
if (CONFIG_FLAG(ForcePostLowerGlobOptInstrString))
226224
{
227-
curr = curr->m_prev;
228-
}
225+
// The instruction has already been lowered, find the start to get the globopt dump
226+
IR::Instr *curr = instr;
227+
while (curr->globOptInstrString == nullptr)
228+
{
229+
curr = curr->m_prev;
230+
}
229231

230-
instr->Dump();
231-
curr->DumpGlobOptInstrString();
232+
instr->Dump();
233+
curr->DumpGlobOptInstrString();
234+
}
232235

233236
AssertMsg(false, "Lazy bailout: bailOutRecord not allocated");
234237
}
@@ -4873,7 +4876,7 @@ LinearScan::ProcessLazyBailOut(IR::Instr *instr)
48734876
// No lazy bailout for function with try/catch for now
48744877
Assert(!this->func->HasTry());
48754878

4876-
this->func->AllocateLazyBailOutRecordArgSlotIfNeeded();
4879+
this->func->EnsureLazyBailOutRecordSlot();
48774880

48784881
if (instr->GetBailOutInfo()->NeedsToRestoreUseOfDst())
48794882
{

0 commit comments

Comments
 (0)