Skip to content

Commit 095a44d

Browse files
MikeHolmanMikeHolman
committed
Fix bugs with InlineArgsOpt
Fixes issue when some inlinees had inlineArgsOpt enabled and some did not Fixes issue handling multiple InlineeEnd
1 parent 4ed51ed commit 095a44d

File tree

13 files changed

+430
-138
lines changed

13 files changed

+430
-138
lines changed

‎lib/Backend/BailOut.cpp

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,8 +1277,6 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
12771277
}
12781278

12791279
// Let's restore the inline stack - so that in case of a stack walk we have it available
1280-
InlinedFrameLayout *inlinedFrameToRestore = nullptr;
1281-
Js::ArgSlot clearedCallInfoCount = 0;
12821280
if (entryPointInfo->HasInlinees())
12831281
{
12841282
InlineeFrameRecord* inlineeFrameRecord = entryPointInfo->FindInlineeFrame(returnAddress);
@@ -1288,20 +1286,14 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
12881286
// object, the cached version (that was previously boxed) will be reused to maintain pointer identity and correctness
12891287
// after the transition to the interpreter.
12901288
InlinedFrameLayout* outerMostFrame = (InlinedFrameLayout *)(((uint8 *)Js::JavascriptCallStackLayout::ToFramePointer(layout)) - entryPointInfo->GetFrameHeight());
1291-
inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, true /*boxArgs*/, &inlinedFrameToRestore, &clearedCallInfoCount);
1289+
inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, true /*boxArgs*/);
12921290
}
12931291
}
12941292

12951293
do
12961294
{
12971295
InlinedFrameLayout *inlinedFrame = (InlinedFrameLayout *)(((char *)layout) + currentBailOutRecord->globalBailOutRecordTable->firstActualStackOffset);
12981296
Js::InlineeCallInfo inlineeCallInfo = inlinedFrame->callInfo;
1299-
if (inlinedFrameToRestore == inlinedFrame)
1300-
{
1301-
// Restore the frame's callinfo count prior to using it to create an interpreter instance
1302-
Assert(inlineeCallInfo.Count == 0);
1303-
inlineeCallInfo.Count = clearedCallInfoCount;
1304-
}
13051297
Assert((Js::ArgSlot)inlineeCallInfo.Count == currentBailOutRecord->actualCount);
13061298

13071299
Js::CallFlags callFlags = Js::CallFlags_Value;

‎lib/Backend/Encoder.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1011,7 +1011,7 @@ void Encoder::RecordInlineeFrame(Func* inlinee, uint32 currentOffset)
10111011
if (!(this->m_func->IsLoopBody() && PHASE_OFF(Js::InlineInJitLoopBodyPhase, this->m_func)) && !this->m_func->IsSimpleJit())
10121012
{
10131013
InlineeFrameRecord* record = nullptr;
1014-
if (inlinee->frameInfo && inlinee->m_hasInlineArgsOpt)
1014+
if (inlinee->frameInfo)
10151015
{
10161016
record = inlinee->frameInfo->record;
10171017
Assert(record != nullptr);

‎lib/Backend/FlowGraph.cpp

Lines changed: 84 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -4488,115 +4488,96 @@ IR::LabelInstr* BasicBlock::CanProveConditionalBranch(IR::BranchInstr *branch, G
44884488
return newTarget;
44894489
}
44904490

4491-
void
4492-
BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
4491+
Value*
4492+
BasicBlock::UpdateValueForCopyTypeInstr(GlobOpt* globOpt, GlobHashTable* localSymToValueMap, IR::Instr* instr)
44934493
{
4494-
IR::LabelInstr * lastBranchTarget = nullptr;
4495-
IR::Instr *currentInlineeEnd = nullptr, *unskippedInlineeEnd = nullptr;
4496-
GlobHashTable * localSymToValueMap = nullptr;
4497-
BVSparse<JitArenaAllocator> * currentPathDefines = nullptr;
4494+
Value* dstValue = nullptr;
4495+
if (instr->m_opcode == Js::OpCode::LdFld)
4496+
{
4497+
// Special handling for LdFld
4498+
Assert(instr->GetSrc1()->IsSymOpnd());
4499+
IR::SymOpnd* symOpnd = instr->GetSrc1()->AsSymOpnd();
44984500

4499-
auto UpdateValueForCopyTypeInstr = [&](IR::Instr *instr) -> Value* {
4500-
Value * dstValue = nullptr;
4501-
if (instr->m_opcode == Js::OpCode::LdFld)
4501+
if (symOpnd->m_sym->IsPropertySym())
45024502
{
4503-
// Special handling for LdFld
4504-
Assert(instr->GetSrc1()->IsSymOpnd());
4505-
IR::SymOpnd *symOpnd = instr->GetSrc1()->AsSymOpnd();
4506-
4507-
if (symOpnd->m_sym->IsPropertySym())
4503+
PropertySym* originalPropertySym = symOpnd->m_sym->AsPropertySym();
4504+
Value* const objectValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, nullptr, originalPropertySym->m_stackSym);
4505+
Sym* objSym = objectValue ? objectValue->GetValueInfo()->GetSymStore() : nullptr;
4506+
PropertySym* prop = PropertySym::Find(objSym ? objSym->m_id : originalPropertySym->m_stackSym->m_id, originalPropertySym->m_propertyId, globOpt->func);
4507+
if (prop)
45084508
{
4509-
PropertySym * originalPropertySym = symOpnd->m_sym->AsPropertySym();
4510-
Value *const objectValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, nullptr, originalPropertySym->m_stackSym);
4511-
Sym* objSym = objectValue ? objectValue->GetValueInfo()->GetSymStore() : nullptr;
4512-
PropertySym *prop = PropertySym::Find(objSym ? objSym->m_id : originalPropertySym->m_stackSym->m_id, originalPropertySym->m_propertyId, globOpt->func);
4513-
if (prop)
4514-
{
4515-
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetStackSym(), prop);
4516-
}
4517-
else
4518-
{
4519-
Value ** localDstValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetStackSym());
4520-
dstValue = *localDstValue = nullptr;
4521-
}
4522-
}
4523-
}
4524-
else if (instr->GetSrc1()->GetStackSym())
4525-
{
4526-
StackSym* src1Sym = instr->GetSrc1()->GetStackSym();
4527-
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetSym(), src1Sym);
4528-
}
4529-
else if (instr->GetSrc1()->IsIntConstOpnd())
4530-
{
4531-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4532-
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsIntConstOpnd()->AsInt32(), instr);
4533-
}
4534-
else if (instr->GetSrc1()->IsInt64ConstOpnd())
4535-
{
4536-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4537-
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsInt64ConstOpnd()->GetValue(), instr);
4538-
}
4539-
else
4540-
{
4541-
ValueType src1Value = instr->GetSrc1()->GetValueType();
4542-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4543-
if (src1Value.IsUndefined() || src1Value.IsBoolean())
4544-
{
4545-
dstValue = *localValue = globOpt->GetVarConstantValue(instr->GetSrc1()->AsAddrOpnd());
4509+
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetStackSym(), prop);
45464510
}
45474511
else
45484512
{
4549-
dstValue = *localValue = nullptr;
4513+
Value** localDstValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetStackSym());
4514+
dstValue = *localDstValue = nullptr;
45504515
}
45514516
}
4552-
return dstValue;
4553-
};
4554-
4555-
FOREACH_INSTR_IN_BLOCK(instr, this)
4517+
}
4518+
else if (instr->GetSrc1()->GetStackSym())
45564519
{
4557-
if (OpCodeAttr::HasDeadFallThrough(instr->m_opcode))
4520+
StackSym* src1Sym = instr->GetSrc1()->GetStackSym();
4521+
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetSym(), src1Sym);
4522+
}
4523+
else if (instr->GetSrc1()->IsIntConstOpnd())
4524+
{
4525+
Value** localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4526+
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsIntConstOpnd()->AsInt32(), instr);
4527+
}
4528+
else if (instr->GetSrc1()->IsInt64ConstOpnd())
4529+
{
4530+
Value** localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4531+
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsInt64ConstOpnd()->GetValue(), instr);
4532+
}
4533+
else
4534+
{
4535+
ValueType src1Value = instr->GetSrc1()->GetValueType();
4536+
Value** localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4537+
if (src1Value.IsUndefined() || src1Value.IsBoolean())
45584538
{
4559-
return;
4539+
dstValue = *localValue = globOpt->GetVarConstantValue(instr->GetSrc1()->AsAddrOpnd());
45604540
}
4561-
if (instr->m_opcode == Js::OpCode::InlineeEnd)
4541+
else
45624542
{
4563-
unskippedInlineeEnd = currentInlineeEnd = instr;
4543+
dstValue = *localValue = nullptr;
45644544
}
4565-
} NEXT_INSTR_IN_BLOCK;
4566-
4567-
IR::Instr * instr = this->GetLastInstr();
4568-
4569-
// We have to first check the legality and only then allocate expensive data structures on the tempArena, because most block will have instructions we cant skip
4545+
}
4546+
return dstValue;
4547+
}
45704548

4549+
bool
4550+
BasicBlock::IsLegalForPathDepBranches(IR::Instr* instr)
4551+
{
45714552
while (instr)
45724553
{
45734554
if (!instr->IsBranchInstr() && !instr->IsLabelInstr() && !IsLegalOpcodeForPathDepBrFold(instr))
45744555
{
4575-
return;
4556+
return false;
45764557
}
45774558
if (instr->IsLabelInstr())
45784559
{
45794560
if (instr->AsLabelInstr()->m_isLoopTop)
45804561
{
45814562
// don't cross over to loops
4582-
return;
4563+
return false;
45834564
}
45844565
}
45854566
if (instr->IsBranchInstr())
45864567
{
4587-
IR::BranchInstr *branch = instr->AsBranchInstr();
4568+
IR::BranchInstr* branch = instr->AsBranchInstr();
45884569
if (branch->IsUnconditional())
45894570
{
45904571
if (!branch->GetTarget())
45914572
{
4592-
return;
4573+
return false;
45934574
}
45944575
instr = branch->GetTarget();
45954576
}
45964577
else
45974578
{
45984579
// Found only legal instructions until a conditional branch, build expensive data structures and check provability
4599-
break;
4580+
return true;
46004581
}
46014582
}
46024583
else
@@ -4605,7 +4586,38 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46054586
}
46064587
}
46074588

4608-
instr = this->GetLastInstr();
4589+
Assert(UNREACHED);
4590+
return false;
4591+
}
4592+
4593+
void
4594+
BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
4595+
{
4596+
IR::LabelInstr * lastBranchTarget = nullptr;
4597+
IR::Instr *currentInlineeEnd = nullptr, *unskippedInlineeEnd = nullptr;
4598+
GlobHashTable * localSymToValueMap = nullptr;
4599+
BVSparse<JitArenaAllocator> * currentPathDefines = nullptr;
4600+
4601+
FOREACH_INSTR_IN_BLOCK(instr, this)
4602+
{
4603+
if (OpCodeAttr::HasDeadFallThrough(instr->m_opcode))
4604+
{
4605+
return;
4606+
}
4607+
if (instr->m_opcode == Js::OpCode::InlineeEnd)
4608+
{
4609+
unskippedInlineeEnd = currentInlineeEnd = instr;
4610+
}
4611+
} NEXT_INSTR_IN_BLOCK;
4612+
4613+
IR::Instr * instr = this->GetLastInstr();
4614+
4615+
// We have to first check the legality and only then allocate expensive data structures on the tempArena, because most block will have instructions we cant skip
4616+
if (!IsLegalForPathDepBranches(instr))
4617+
{
4618+
return;
4619+
}
4620+
46094621
// Allocate hefty structures, we will not free them because OptBlock does a Reset on the tempAlloc
46104622
localSymToValueMap = GlobHashTable::New(globOpt->tempAlloc, 8);
46114623
currentPathDefines = JitAnew(globOpt->tempAlloc, BVSparse<JitArenaAllocator>, globOpt->tempAlloc);
@@ -4653,7 +4665,7 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46534665

46544666
if (IsCopyTypeInstr(instr))
46554667
{
4656-
Value *dstValue = UpdateValueForCopyTypeInstr(instr);
4668+
Value *dstValue = UpdateValueForCopyTypeInstr(globOpt, localSymToValueMap, instr);
46574669
if (instr->m_opcode == Js::OpCode::LdFld && !dstValue)
46584670
{
46594671
// We cannot skip a LdFld if we didnt find its valueInfo in the localValueTable

‎lib/Backend/FlowGraph.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,8 @@ class BasicBlock
352352
void MergePredBlocksValueMaps(GlobOpt* globOptState);
353353
private:
354354
void CleanUpValueMaps();
355+
Value* UpdateValueForCopyTypeInstr(GlobOpt* globOpt, GlobHashTable* localSymToValueMap, IR::Instr* instr);
356+
static bool IsLegalForPathDepBranches(IR::Instr* instr);
355357
void CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt);
356358
Value * FindValueInLocalThenGlobalValueTableAndUpdate(GlobOpt *globOpt, GlobHashTable * localSymToValueMap, IR::Instr *instr, Sym *dstSym, Sym *srcSym);
357359
IR::LabelInstr* CanProveConditionalBranch(IR::BranchInstr *branch, GlobOpt* globOpt, GlobHashTable * localSymToValueMap);

‎lib/Backend/GlobOpt.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -870,6 +870,7 @@ class GlobOpt
870870
void ProcessInlineeEnd(IR::Instr * instr);
871871
void TrackCalls(IR::Instr * instr);
872872
void RecordInlineeFrameInfo(IR::Instr* instr);
873+
void ClearInlineeFrameInfo(IR::Instr* instr);
873874
void EndTrackCall(IR::Instr * instr);
874875
void EndTrackingOfArgObjSymsForInlinee();
875876
void FillBailOutInfo(BasicBlock *block, BailOutInfo *bailOutInfo);

‎lib/Backend/GlobOptBailOut.cpp

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -482,17 +482,29 @@ GlobOpt::CaptureByteCodeSymUses(IR::Instr * instr)
482482
void
483483
GlobOpt::ProcessInlineeEnd(IR::Instr* instr)
484484
{
485-
if (!PHASE_OFF(Js::StackArgLenConstOptPhase, instr->m_func) &&
485+
if (!PHASE_OFF(Js::StackArgLenConstOptPhase, instr->m_func) &&
486486
!IsLoopPrePass() &&
487-
(!instr->m_func->GetJITFunctionBody()->UsesArgumentsObject() || instr->m_func->IsStackArgsEnabled()) &&
488-
instr->m_func->unoptimizableArgumentsObjReference == 0 && instr->m_func->unoptimizableArgumentsObjReferenceInInlinees == 0)
487+
(!instr->m_func->GetJITFunctionBody()->UsesArgumentsObject() || instr->m_func->IsStackArgsEnabled()))
489488
{
490-
instr->m_func->hasUnoptimizedArgumentsAccess = false;
491-
if (!instr->m_func->m_hasInlineArgsOpt && DoInlineArgsOpt(instr->m_func))
489+
if (instr->m_func->unoptimizableArgumentsObjReference == 0 && instr->m_func->unoptimizableArgumentsObjReferenceInInlinees == 0)
492490
{
493-
instr->m_func->m_hasInlineArgsOpt = true;
494-
Assert(instr->m_func->cachedInlineeFrameInfo);
495-
instr->m_func->frameInfo = instr->m_func->cachedInlineeFrameInfo;
491+
instr->m_func->hasUnoptimizedArgumentsAccess = false;
492+
if (!instr->m_func->m_hasInlineArgsOpt && DoInlineArgsOpt(instr->m_func))
493+
{
494+
instr->m_func->m_hasInlineArgsOpt = true;
495+
Assert(instr->m_func->cachedInlineeFrameInfo);
496+
instr->m_func->frameInfo = instr->m_func->cachedInlineeFrameInfo;
497+
}
498+
}
499+
else
500+
{
501+
instr->m_func->hasUnoptimizedArgumentsAccess = true;
502+
503+
if (instr->m_func->m_hasInlineArgsOpt && instr->m_func->cachedInlineeFrameInfo)
504+
{
505+
instr->m_func->m_hasInlineArgsOpt = false;
506+
ClearInlineeFrameInfo(instr);
507+
}
496508
}
497509
}
498510

@@ -768,6 +780,24 @@ GlobOpt::TrackCalls(IR::Instr * instr)
768780
}
769781
}
770782

783+
void GlobOpt::ClearInlineeFrameInfo(IR::Instr* inlineeEnd)
784+
{
785+
if (this->IsLoopPrePass())
786+
{
787+
return;
788+
}
789+
790+
InlineeFrameInfo* frameInfo = inlineeEnd->m_func->frameInfo;
791+
inlineeEnd->m_func->frameInfo = nullptr;
792+
793+
if (!frameInfo || !frameInfo->isRecorded)
794+
{
795+
return;
796+
}
797+
frameInfo->function = InlineFrameInfoValue();
798+
frameInfo->arguments->Clear();
799+
}
800+
771801
void GlobOpt::RecordInlineeFrameInfo(IR::Instr* inlineeEnd)
772802
{
773803
if (this->IsLoopPrePass())

0 commit comments

Comments
 (0)