Skip to content

Commit 9d90278

Browse files
MikeHolmanMikeHolman
committed
[MERGE #6213 @MikeHolman] Fix bugs with InlineArgsOpt
Merge pull request #6213 from MikeHolman:inlineargsparent Fixes issue when some inlinees had inlineArgsOpt enabled and some did not Fixes issue handling multiple InlineeEnd OS: 21628964
2 parents 74218d5 + bed8b77 commit 9d90278

16 files changed

+435
-140
lines changed

‎lib/Backend/BackwardPass.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8087,7 +8087,6 @@ BackwardPass::ProcessInlineeStart(IR::Instr* inlineeStart)
80878087
inlineeStart->m_func->SetFirstArgOffset(inlineeStart);
80888088

80898089
IR::Instr* startCallInstr = nullptr;
8090-
bool noImplicitCallsInInlinee = false;
80918090
// Inlinee has no bailouts or implicit calls. Get rid of the inline overhead.
80928091
auto removeInstr = [&](IR::Instr* argInstr)
80938092
{
@@ -8109,7 +8108,6 @@ BackwardPass::ProcessInlineeStart(IR::Instr* inlineeStart)
81098108
// If there are no implicit calls - bailouts/throws - we can remove all inlining overhead.
81108109
if (!inlineeStart->m_func->GetHasImplicitCalls())
81118110
{
8112-
noImplicitCallsInInlinee = true;
81138111
inlineeStart->IterateArgInstrs(removeInstr);
81148112

81158113
inlineeStart->IterateMetaArgs([](IR::Instr* metArg)
@@ -8118,6 +8116,7 @@ BackwardPass::ProcessInlineeStart(IR::Instr* inlineeStart)
81188116
return false;
81198117
});
81208118
inlineeStart->m_func->m_hasInlineArgsOpt = false;
8119+
inlineeStart->m_func->m_hasInlineOverheadRemoved = true;
81218120
removeInstr(inlineeStart);
81228121
return true;
81238122
}

‎lib/Backend/BailOut.cpp

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

12921292
// Let's restore the inline stack - so that in case of a stack walk we have it available
1293-
InlinedFrameLayout *inlinedFrameToRestore = nullptr;
1294-
Js::ArgSlot clearedCallInfoCount = 0;
12951293
if (entryPointInfo->HasInlinees())
12961294
{
12971295
InlineeFrameRecord* inlineeFrameRecord = entryPointInfo->FindInlineeFrame(returnAddress);
@@ -1301,20 +1299,14 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
13011299
// object, the cached version (that was previously boxed) will be reused to maintain pointer identity and correctness
13021300
// after the transition to the interpreter.
13031301
InlinedFrameLayout* outerMostFrame = (InlinedFrameLayout *)(((uint8 *)Js::JavascriptCallStackLayout::ToFramePointer(layout)) - entryPointInfo->GetFrameHeight());
1304-
inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, true /*boxArgs*/, &inlinedFrameToRestore, &clearedCallInfoCount);
1302+
inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, true /*boxArgs*/);
13051303
}
13061304
}
13071305

13081306
do
13091307
{
13101308
InlinedFrameLayout *inlinedFrame = (InlinedFrameLayout *)(((char *)layout) + currentBailOutRecord->globalBailOutRecordTable->firstActualStackOffset);
13111309
Js::InlineeCallInfo inlineeCallInfo = inlinedFrame->callInfo;
1312-
if (inlinedFrameToRestore == inlinedFrame)
1313-
{
1314-
// Restore the frame's callinfo count prior to using it to create an interpreter instance
1315-
Assert(inlineeCallInfo.Count == 0);
1316-
inlineeCallInfo.Count = clearedCallInfoCount;
1317-
}
13181310
Assert((Js::ArgSlot)inlineeCallInfo.Count == currentBailOutRecord->actualCount);
13191311

13201312
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
@@ -4489,115 +4489,96 @@ IR::LabelInstr* BasicBlock::CanProveConditionalBranch(IR::BranchInstr *branch, G
44894489
return newTarget;
44904490
}
44914491

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

4500-
auto UpdateValueForCopyTypeInstr = [&](IR::Instr *instr) -> Value* {
4501-
Value * dstValue = nullptr;
4502-
if (instr->m_opcode == Js::OpCode::LdFld)
4502+
if (symOpnd->m_sym->IsPropertySym())
45034503
{
4504-
// Special handling for LdFld
4505-
Assert(instr->GetSrc1()->IsSymOpnd());
4506-
IR::SymOpnd *symOpnd = instr->GetSrc1()->AsSymOpnd();
4507-
4508-
if (symOpnd->m_sym->IsPropertySym())
4504+
PropertySym* originalPropertySym = symOpnd->m_sym->AsPropertySym();
4505+
Value* const objectValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, nullptr, originalPropertySym->m_stackSym);
4506+
Sym* objSym = objectValue ? objectValue->GetValueInfo()->GetSymStore() : nullptr;
4507+
PropertySym* prop = PropertySym::Find(objSym ? objSym->m_id : originalPropertySym->m_stackSym->m_id, originalPropertySym->m_propertyId, globOpt->func);
4508+
if (prop)
45094509
{
4510-
PropertySym * originalPropertySym = symOpnd->m_sym->AsPropertySym();
4511-
Value *const objectValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, nullptr, originalPropertySym->m_stackSym);
4512-
Sym* objSym = objectValue ? objectValue->GetValueInfo()->GetSymStore() : nullptr;
4513-
PropertySym *prop = PropertySym::Find(objSym ? objSym->m_id : originalPropertySym->m_stackSym->m_id, originalPropertySym->m_propertyId, globOpt->func);
4514-
if (prop)
4515-
{
4516-
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetStackSym(), prop);
4517-
}
4518-
else
4519-
{
4520-
Value ** localDstValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetStackSym());
4521-
dstValue = *localDstValue = nullptr;
4522-
}
4523-
}
4524-
}
4525-
else if (instr->GetSrc1()->GetStackSym())
4526-
{
4527-
StackSym* src1Sym = instr->GetSrc1()->GetStackSym();
4528-
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetSym(), src1Sym);
4529-
}
4530-
else if (instr->GetSrc1()->IsIntConstOpnd())
4531-
{
4532-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4533-
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsIntConstOpnd()->AsInt32(), instr);
4534-
}
4535-
else if (instr->GetSrc1()->IsInt64ConstOpnd())
4536-
{
4537-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4538-
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsInt64ConstOpnd()->GetValue(), instr);
4539-
}
4540-
else
4541-
{
4542-
ValueType src1Value = instr->GetSrc1()->GetValueType();
4543-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4544-
if (src1Value.IsUndefined() || src1Value.IsBoolean())
4545-
{
4546-
dstValue = *localValue = globOpt->GetVarConstantValue(instr->GetSrc1()->AsAddrOpnd());
4510+
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetStackSym(), prop);
45474511
}
45484512
else
45494513
{
4550-
dstValue = *localValue = nullptr;
4514+
Value** localDstValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetStackSym());
4515+
dstValue = *localDstValue = nullptr;
45514516
}
45524517
}
4553-
return dstValue;
4554-
};
4555-
4556-
FOREACH_INSTR_IN_BLOCK(instr, this)
4518+
}
4519+
else if (instr->GetSrc1()->GetStackSym())
45574520
{
4558-
if (OpCodeAttr::HasDeadFallThrough(instr->m_opcode))
4521+
StackSym* src1Sym = instr->GetSrc1()->GetStackSym();
4522+
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetSym(), src1Sym);
4523+
}
4524+
else if (instr->GetSrc1()->IsIntConstOpnd())
4525+
{
4526+
Value** localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4527+
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsIntConstOpnd()->AsInt32(), instr);
4528+
}
4529+
else if (instr->GetSrc1()->IsInt64ConstOpnd())
4530+
{
4531+
Value** localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4532+
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsInt64ConstOpnd()->GetValue(), instr);
4533+
}
4534+
else
4535+
{
4536+
ValueType src1Value = instr->GetSrc1()->GetValueType();
4537+
Value** localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4538+
if (src1Value.IsUndefined() || src1Value.IsBoolean())
45594539
{
4560-
return;
4540+
dstValue = *localValue = globOpt->GetVarConstantValue(instr->GetSrc1()->AsAddrOpnd());
45614541
}
4562-
if (instr->m_opcode == Js::OpCode::InlineeEnd)
4542+
else
45634543
{
4564-
unskippedInlineeEnd = currentInlineeEnd = instr;
4544+
dstValue = *localValue = nullptr;
45654545
}
4566-
} NEXT_INSTR_IN_BLOCK;
4567-
4568-
IR::Instr * instr = this->GetLastInstr();
4569-
4570-
// 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
4546+
}
4547+
return dstValue;
4548+
}
45714549

4550+
bool
4551+
BasicBlock::IsLegalForPathDepBranches(IR::Instr* instr)
4552+
{
45724553
while (instr)
45734554
{
45744555
if (!instr->IsBranchInstr() && !instr->IsLabelInstr() && !IsLegalOpcodeForPathDepBrFold(instr))
45754556
{
4576-
return;
4557+
return false;
45774558
}
45784559
if (instr->IsLabelInstr())
45794560
{
45804561
if (instr->AsLabelInstr()->m_isLoopTop)
45814562
{
45824563
// don't cross over to loops
4583-
return;
4564+
return false;
45844565
}
45854566
}
45864567
if (instr->IsBranchInstr())
45874568
{
4588-
IR::BranchInstr *branch = instr->AsBranchInstr();
4569+
IR::BranchInstr* branch = instr->AsBranchInstr();
45894570
if (branch->IsUnconditional())
45904571
{
45914572
if (!branch->GetTarget())
45924573
{
4593-
return;
4574+
return false;
45944575
}
45954576
instr = branch->GetTarget();
45964577
}
45974578
else
45984579
{
45994580
// Found only legal instructions until a conditional branch, build expensive data structures and check provability
4600-
break;
4581+
return true;
46014582
}
46024583
}
46034584
else
@@ -4606,7 +4587,38 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46064587
}
46074588
}
46084589

4609-
instr = this->GetLastInstr();
4590+
Assert(UNREACHED);
4591+
return false;
4592+
}
4593+
4594+
void
4595+
BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
4596+
{
4597+
IR::LabelInstr * lastBranchTarget = nullptr;
4598+
IR::Instr *currentInlineeEnd = nullptr, *unskippedInlineeEnd = nullptr;
4599+
GlobHashTable * localSymToValueMap = nullptr;
4600+
BVSparse<JitArenaAllocator> * currentPathDefines = nullptr;
4601+
4602+
FOREACH_INSTR_IN_BLOCK(instr, this)
4603+
{
4604+
if (OpCodeAttr::HasDeadFallThrough(instr->m_opcode))
4605+
{
4606+
return;
4607+
}
4608+
if (instr->m_opcode == Js::OpCode::InlineeEnd)
4609+
{
4610+
unskippedInlineeEnd = currentInlineeEnd = instr;
4611+
}
4612+
} NEXT_INSTR_IN_BLOCK;
4613+
4614+
IR::Instr * instr = this->GetLastInstr();
4615+
4616+
// 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
4617+
if (!IsLegalForPathDepBranches(instr))
4618+
{
4619+
return;
4620+
}
4621+
46104622
// Allocate hefty structures, we will not free them because OptBlock does a Reset on the tempAlloc
46114623
localSymToValueMap = GlobHashTable::New(globOpt->tempAlloc, 8);
46124624
currentPathDefines = JitAnew(globOpt->tempAlloc, BVSparse<JitArenaAllocator>, globOpt->tempAlloc);
@@ -4654,7 +4666,7 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46544666

46554667
if (IsCopyTypeInstr(instr))
46564668
{
4657-
Value *dstValue = UpdateValueForCopyTypeInstr(instr);
4669+
Value *dstValue = UpdateValueForCopyTypeInstr(globOpt, localSymToValueMap, instr);
46584670
if (instr->m_opcode == Js::OpCode::LdFld && !dstValue)
46594671
{
46604672
// 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/Func.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
6464
m_argSlotsForFunctionsCalled(0),
6565
m_hasCalls(false),
6666
m_hasInlineArgsOpt(false),
67+
m_hasInlineOverheadRemoved(false),
6768
m_canDoInlineArgsOpt(true),
6869
unoptimizableArgumentsObjReference(0),
6970
unoptimizableArgumentsObjReferenceInInlinees(0),

‎lib/Backend/Func.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -691,6 +691,7 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
691691
InlineeFrameInfo* cachedInlineeFrameInfo;
692692
bool m_hasCalls: 1; // This is more accurate compared to m_isLeaf
693693
bool m_hasInlineArgsOpt : 1;
694+
bool m_hasInlineOverheadRemoved : 1;
694695
bool m_doFastPaths : 1;
695696
bool hasBailout: 1;
696697
bool hasBailoutInEHRegion : 1;

‎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)