Skip to content

Commit 825849b

Browse files
nhat-nguyennhat-nguyen
committed
[MERGE #6170 @nhat-nguyen] Enable basic jitting without global optimizer for generators on x64
Merge pull request #6170 from nhat-nguyen:generator - Cleaned up the bail-in code and the IR dumps with self-descriptive label names and added special opcodes just for generators; this made it easier to read the dumps in general and to perform some other logic using those opcodes - Fixed epilogues in generators. We now have 2 epilogue labels in generators that can be jumped to. The first one nulls out the interpreter frame to signal generator completion and restores all callee-save registers - used for return statements. The other one only does the latter, used for jumps after bailing out. (the register restore part is shared between the 2 labels) - We also already have the mechanism to deal with yield points that were optimized away (called `BailOutForElidedYield`), I also changed the logic a bit to work with these new epilogue labels - The resume jump table must be placed close enough to the beginning of the jit'd function so that we don't accidentally re-run the code, but also further below enough so that we can reload "global" values such as constants / environment (right now, it is placed right after we load the environment). One thing to note is that objects like Environment are always loaded from the `ScriptFunction`, but some like the `FrameDisplay/LocalClosure` might be created on the heap and transferred over to the interpreter frame afterwards. So we want to place the jump table before anything that can created new objects on the heap - also made sure that the bail-in code doesn't blindly overwrite any of the values we reload before the jump table - Right now the bail-in code does the reverse of what the bail-out code does: grabbing all those special field members and putting them back, the rest of the bytecode symbols are simply loaded from the usual `localSlots` space - Made for-in enumerators work with generators by always using the space allocated on the interpreter frame - Previously, in some cases, we would not have an interpreter frame the first time we are in the jit'd code and only create the frame once we bail out for the first time. This is fine for normal generators because we would bail-out to return the generator object, so the next time the interpreter frame would have already been created. However, `async` functions don't have this first yield point, so we cannot use the interpreter frame space to store the enumerators. - to fix this, we would always create an interpreter frame in the jit if the generator doesn't already have one - Since the bail-in code relies on rax and rcx to do the reload, sometimes we would accidentally overwrite symbols reloaded before the jump table that might already be in those two registers. I made sure that we save/restore rax/rcx in the bail-in code if there are such symbols
2 parents bba835d + 2b9c17d commit 825849b

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+1346
-534
lines changed

‎lib/Backend/BackendApi.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ Js::JavascriptMethod GetCheckAsmJsCodeGenThunk()
126126

127127
uint GetBailOutRegisterSaveSlotCount()
128128
{
129-
// REVIEW: not all registers are used, we are allocating more space then necessary.
129+
// REVIEW: not all registers are used, we are allocating more space than necessary.
130130
return LinearScanMD::GetRegisterSaveSlotCount();
131131
}
132132

‎lib/Backend/BackwardPass.cpp

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,20 +88,26 @@ BackwardPass::DoMarkTempNumbers() const
8888
}
8989

9090
bool
91-
BackwardPass::DoMarkTempObjects() const
92-
{
93-
// only mark temp object on the backward store phase
94-
return (tag == Js::BackwardPhase) && !PHASE_OFF(Js::MarkTempPhase, this->func) &&
95-
!PHASE_OFF(Js::MarkTempObjectPhase, this->func) && func->DoGlobOpt() && func->GetHasTempObjectProducingInstr() &&
96-
!func->IsJitInDebugMode() &&
97-
func->DoGlobOptsForGeneratorFunc();
91+
BackwardPass::SatisfyMarkTempObjectsConditions() const {
92+
return !PHASE_OFF(Js::MarkTempPhase, this->func) &&
93+
!PHASE_OFF(Js::MarkTempObjectPhase, this->func) &&
94+
func->DoGlobOpt() && func->GetHasTempObjectProducingInstr() &&
95+
!func->IsJitInDebugMode() &&
96+
func->DoGlobOptsForGeneratorFunc();
9897

9998
// Why MarkTempObject is disabled under debugger:
10099
// We add 'identified so far dead non-temp locals' to byteCodeUpwardExposedUsed in ProcessBailOutInfo,
101100
// this may cause MarkTempObject to convert some temps back to non-temp when it sees a 'transferred exposed use'
102101
// from a temp to non-temp. That's in general not a supported conversion (while non-temp -> temp is fine).
103102
}
104103

104+
bool
105+
BackwardPass::DoMarkTempObjects() const
106+
{
107+
// only mark temp object on the backward store phase
108+
return (tag == Js::BackwardPhase) && SatisfyMarkTempObjectsConditions();
109+
}
110+
105111
bool
106112
BackwardPass::DoMarkTempNumbersOnTempObjects() const
107113
{
@@ -113,8 +119,7 @@ bool
113119
BackwardPass::DoMarkTempObjectVerify() const
114120
{
115121
// only mark temp object on the backward store phase
116-
return (tag == Js::DeadStorePhase) && !PHASE_OFF(Js::MarkTempPhase, this->func) &&
117-
!PHASE_OFF(Js::MarkTempObjectPhase, this->func) && func->DoGlobOpt() && func->GetHasTempObjectProducingInstr();
122+
return (tag == Js::DeadStorePhase) && SatisfyMarkTempObjectsConditions();
118123
}
119124
#endif
120125

@@ -7709,7 +7714,7 @@ BackwardPass::ProcessDef(IR::Opnd * opnd)
77097714
PropertySym *propertySym = sym->AsPropertySym();
77107715
ProcessStackSymUse(propertySym->m_stackSym, isJITOptimizedReg);
77117716

7712-
if(IsCollectionPass())
7717+
if (IsCollectionPass())
77137718
{
77147719
return false;
77157720
}
@@ -7796,7 +7801,7 @@ BackwardPass::ProcessDef(IR::Opnd * opnd)
77967801
}
77977802
}
77987803

7799-
if(IsCollectionPass())
7804+
if (IsCollectionPass())
78007805
{
78017806
return false;
78027807
}
@@ -8247,9 +8252,20 @@ BackwardPass::ProcessBailOnNoProfile(IR::Instr *instr, BasicBlock *block)
82478252
return false;
82488253
}
82498254

8255+
// For generator functions, we don't want to move the BailOutOnNoProfile above
8256+
// certain instructions such as ResumeYield/ResumeYieldStar/CreateInterpreterStackFrameForGenerator
8257+
// This indicates the insertion point for the BailOutOnNoProfile in such cases.
8258+
IR::Instr *insertionPointForGenerator = nullptr;
8259+
82508260
// Don't hoist if we see calls with profile data (recursive calls)
82518261
while(!curInstr->StartsBasicBlock())
82528262
{
8263+
if (curInstr->DontHoistBailOnNoProfileAboveInGeneratorFunction())
8264+
{
8265+
Assert(insertionPointForGenerator == nullptr);
8266+
insertionPointForGenerator = curInstr;
8267+
}
8268+
82538269
// If a function was inlined, it must have had profile info.
82548270
if (curInstr->m_opcode == Js::OpCode::InlineeEnd || curInstr->m_opcode == Js::OpCode::InlineBuiltInEnd || curInstr->m_opcode == Js::OpCode::InlineNonTrackingBuiltInEnd
82558271
|| curInstr->m_opcode == Js::OpCode::InlineeStart || curInstr->m_opcode == Js::OpCode::EndCallForPolymorphicInlinee)
@@ -8320,7 +8336,8 @@ BackwardPass::ProcessBailOnNoProfile(IR::Instr *instr, BasicBlock *block)
83208336
// Now try to move this up the flowgraph to the predecessor blocks
83218337
FOREACH_PREDECESSOR_BLOCK(pred, block)
83228338
{
8323-
bool hoistBailToPred = true;
8339+
// Don't hoist BailOnNoProfile up past blocks containing ResumeYield/ResumeYieldStar
8340+
bool hoistBailToPred = (insertionPointForGenerator == nullptr);
83248341

83258342
if (block->isLoopHeader && pred->loop == block->loop)
83268343
{
@@ -8396,10 +8413,19 @@ BackwardPass::ProcessBailOnNoProfile(IR::Instr *instr, BasicBlock *block)
83968413
#if DBG
83978414
blockHeadInstr->m_noHelperAssert = true;
83988415
#endif
8399-
block->beginsBailOnNoProfile = true;
84008416

84018417
instr->m_func = curInstr->m_func;
8402-
curInstr->InsertAfter(instr);
8418+
8419+
if (insertionPointForGenerator != nullptr)
8420+
{
8421+
insertionPointForGenerator->InsertAfter(instr);
8422+
block->beginsBailOnNoProfile = false;
8423+
}
8424+
else
8425+
{
8426+
curInstr->InsertAfter(instr);
8427+
block->beginsBailOnNoProfile = true;
8428+
}
84038429

84048430
bool setLastInstr = (curInstr == block->GetLastInstr());
84058431
if (setLastInstr)

‎lib/Backend/BackwardPass.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ class BackwardPass
118118
bool DoByteCodeUpwardExposedUsed() const;
119119
bool DoCaptureByteCodeUpwardExposedUsed() const;
120120
void DoSetDead(IR::Opnd * opnd, bool isDead) const;
121+
122+
bool SatisfyMarkTempObjectsConditions() const;
121123
bool DoMarkTempObjects() const;
122124
bool DoMarkTempNumbers() const;
123125
bool DoMarkTempNumbersOnTempObjects() const;

‎lib/Backend/BailOut.cpp

Lines changed: 37 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,25 @@ BailOutRecord::RestoreValue(IR::BailOutKind bailOutKind, Js::JavascriptCallStack
989989
value = Js::JavascriptNumber::ToVar(int32Value, scriptContext);
990990
BAILOUT_VERBOSE_TRACE(newInstance->function->GetFunctionBody(), bailOutKind, _u(", value: %10d (ToVar: 0x%p)"), int32Value, value);
991991
}
992+
else if (regSlot == newInstance->function->GetFunctionBody()->GetYieldRegister() && newInstance->function->GetFunctionBody()->IsCoroutine())
993+
{
994+
// This value can only either be:
995+
// 1) the ResumeYieldData. Even though this value is on the stack, it is only used to extract the data as part of Op_ResumeYield.
996+
// So there is no need to box the value.
997+
// 2) the object used as the return value for yield statement. This object is created on the heap, so no need to box either.
998+
Assert(value);
999+
1000+
#if ENABLE_DEBUG_CONFIG_OPTIONS
1001+
if (ThreadContext::IsOnStack(value))
1002+
{
1003+
BAILOUT_VERBOSE_TRACE(newInstance->function->GetFunctionBody(), bailOutKind, _u(", value: 0x%p (ResumeYieldData)"), value);
1004+
}
1005+
else
1006+
{
1007+
BAILOUT_VERBOSE_TRACE(newInstance->function->GetFunctionBody(), bailOutKind, _u(", value: 0x%p (Yield Return Value)"), value);
1008+
}
1009+
#endif
1010+
}
9921011
else
9931012
{
9941013
BAILOUT_VERBOSE_TRACE(newInstance->function->GetFunctionBody(), bailOutKind, _u(", value: 0x%p"), value);
@@ -1507,63 +1526,32 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
15071526
if (executeFunction->IsCoroutine())
15081527
{
15091528
// If the FunctionBody is a generator then this call is being made by one of the three
1510-
// generator resuming methods: next(), throw(), or return(). They all pass the generator
1511-
// object as the first of two arguments. The real user arguments are obtained from the
1512-
// generator object. The second argument is the ResumeYieldData which is only needed
1513-
// when resuming a generator and not needed when yielding from a generator, as is occurring
1514-
// here.
1529+
// generator resuming methods: next(), throw(), or return(). They all pass the generator
1530+
// object as the first of two arguments. The real user arguments are obtained from the
1531+
// generator object. The second argument is the ResumeYieldData which is only needed when
1532+
// resuming a generator and not needed when yielding from a generator, as is occurring here.
15151533
AssertMsg(args.Info.Count == 2, "Generator ScriptFunctions should only be invoked by generator APIs with the pair of arguments they pass in -- the generator object and a ResumeYieldData pointer");
15161534
Js::JavascriptGenerator* generator = Js::VarTo<Js::JavascriptGenerator>(args[0]);
15171535
newInstance = generator->GetFrame();
15181536

1519-
if (newInstance != nullptr)
1520-
{
1521-
// BailOut will recompute OutArg pointers based on BailOutRecord. Reset them back
1522-
// to initial position before that happens so that OP_StartCall calls don't accumulate
1523-
// incorrectly over multiple yield bailouts.
1524-
newInstance->ResetOut();
1525-
1526-
// The debugger relies on comparing stack addresses of frames to decide when a step_out is complete so
1527-
// give the InterpreterStackFrame a legit enough stack address to make this comparison work.
1528-
newInstance->m_stackAddress = reinterpret_cast<DWORD_PTR>(&generator);
1529-
}
1530-
else
1531-
{
1532-
//
1533-
// Allocate a new InterpreterStackFrame instance on the recycler heap.
1534-
// It will live with the JavascriptGenerator object.
1535-
//
1536-
Js::Arguments generatorArgs = generator->GetArguments();
1537-
Js::InterpreterStackFrame::Setup setup(function, generatorArgs, true, isInlinee);
1538-
Assert(setup.GetStackAllocationVarCount() == 0);
1539-
size_t varAllocCount = setup.GetAllocationVarCount();
1540-
size_t varSizeInBytes = varAllocCount * sizeof(Js::Var);
1541-
DWORD_PTR stackAddr = reinterpret_cast<DWORD_PTR>(&generator); // as mentioned above, use any stack address from this frame to ensure correct debugging functionality
1542-
Js::LoopHeader* loopHeaderArray = executeFunction->GetHasAllocatedLoopHeaders() ? executeFunction->GetLoopHeaderArrayPtr() : nullptr;
1543-
1544-
allocation = RecyclerNewPlus(functionScriptContext->GetRecycler(), varSizeInBytes, Js::Var);
1545-
1546-
// Initialize the interpreter stack frame (constants) but not the param, the bailout record will restore the value
1547-
#if DBG
1548-
// Allocate invalidVar on GC instead of stack since this InterpreterStackFrame will out live the current real frame
1549-
Js::Var invalidVar = (Js::RecyclableObject*)RecyclerNewPlusLeaf(functionScriptContext->GetRecycler(), sizeof(Js::RecyclableObject), Js::Var);
1550-
memset(invalidVar, 0xFE, sizeof(Js::RecyclableObject));
1551-
#endif
1537+
// The jit relies on the interpreter stack frame to store various information such as
1538+
// for-in enumerators. Therefore, we always create an interpreter stack frame for generator
1539+
// as part of the resume jump table, at the beginning of the jit'd function, if it doesn't
1540+
// already exist.
1541+
Assert(newInstance != nullptr);
15521542

1553-
newInstance = setup.InitializeAllocation(allocation, nullptr, false, false, loopHeaderArray, stackAddr
1554-
#if DBG
1555-
, invalidVar
1556-
#endif
1557-
);
1543+
// BailOut will recompute OutArg pointers based on BailOutRecord. Reset them back
1544+
// to initial position before that happens so that OP_StartCall calls don't accumulate
1545+
// incorrectly over multiple yield bailouts.
1546+
newInstance->ResetOut();
15581547

1559-
newInstance->m_reader.Create(executeFunction);
1560-
1561-
generator->SetFrame(newInstance, varSizeInBytes);
1562-
}
1548+
// The debugger relies on comparing stack addresses of frames to decide when a step_out is complete so
1549+
// give the InterpreterStackFrame a legit enough stack address to make this comparison work.
1550+
newInstance->m_stackAddress = reinterpret_cast<DWORD_PTR>(&generator);
15631551
}
15641552
else
15651553
{
1566-
Js::InterpreterStackFrame::Setup setup(function, args, true, isInlinee);
1554+
Js::InterpreterStackFrame::Setup setup(function, args, true /* bailedOut */, isInlinee);
15671555
size_t varAllocCount = setup.GetAllocationVarCount();
15681556
size_t stackVarAllocCount = setup.GetStackAllocationVarCount();
15691557
size_t varSizeInBytes;
@@ -2826,7 +2814,7 @@ void BailOutRecord::CheckPreemptiveRejit(Js::FunctionBody* executeFunction, IR::
28262814

28272815
Js::Var BailOutRecord::BailOutForElidedYield(void * framePointer)
28282816
{
2829-
JIT_HELPER_REENTRANT_HEADER(NoSaveRegistersBailOutForElidedYield);
2817+
JIT_HELPER_NOT_REENTRANT_NOLOCK_HEADER(NoSaveRegistersBailOutForElidedYield);
28302818
Js::JavascriptCallStackLayout * const layout = Js::JavascriptCallStackLayout::FromFramePointer(framePointer);
28312819
Js::ScriptFunction ** functionRef = (Js::ScriptFunction **)&layout->functionObject;
28322820
Js::ScriptFunction * function = *functionRef;

‎lib/Backend/FlowGraph.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3318,7 +3318,7 @@ FlowGraph::RemoveInstr(IR::Instr *instr, GlobOpt * globOpt)
33183318
if (opcode == Js::OpCode::Yield)
33193319
{
33203320
IR::Instr *instrLabel = newByteCodeUseInstr->m_next;
3321-
while (instrLabel->m_opcode != Js::OpCode::Label)
3321+
while (instrLabel->m_opcode != Js::OpCode::GeneratorBailInLabel)
33223322
{
33233323
instrLabel = instrLabel->m_next;
33243324
}

‎lib/Backend/Func.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
148148
, m_forInEnumeratorArrayOffset(-1)
149149
, argInsCount(0)
150150
, m_globalObjTypeSpecFldInfoArray(nullptr)
151+
, m_forInEnumeratorForGeneratorSym(nullptr)
151152
#if LOWER_SPLIT_INT64
152153
, m_int64SymPairMap(nullptr)
153154
#endif

‎lib/Backend/Func.h

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,8 @@ class Func
205205
return
206206
!PHASE_OFF(Js::GlobOptPhase, this) && !IsSimpleJit() &&
207207
(!GetTopFunc()->HasTry() || GetTopFunc()->CanOptimizeTryCatch()) &&
208-
(!GetTopFunc()->HasFinally() || GetTopFunc()->CanOptimizeTryFinally());
208+
(!GetTopFunc()->HasFinally() || GetTopFunc()->CanOptimizeTryFinally()) &&
209+
!GetTopFunc()->GetJITFunctionBody()->IsCoroutine();
209210
}
210211

211212
bool DoInline() const
@@ -1061,11 +1062,23 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
10611062
StackSym* m_loopParamSym;
10621063
StackSym* m_bailoutReturnValueSym;
10631064
StackSym* m_hasBailedOutSym;
1065+
StackSym* m_forInEnumeratorForGeneratorSym;
10641066

10651067
public:
10661068
StackSym* tempSymDouble;
10671069
StackSym* tempSymBool;
10681070

1071+
void SetForInEnumeratorSymForGeneratorSym(StackSym* sym)
1072+
{
1073+
Assert(this->m_forInEnumeratorForGeneratorSym == nullptr);
1074+
this->m_forInEnumeratorForGeneratorSym = sym;
1075+
}
1076+
1077+
StackSym* GetForInEnumeratorSymForGeneratorSym() const
1078+
{
1079+
return this->m_forInEnumeratorForGeneratorSym;
1080+
}
1081+
10691082
// StackSyms' corresponding getters/setters
10701083
void SetInlineeFrameStartSym(StackSym* sym)
10711084
{

‎lib/Backend/GlobOpt.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4704,6 +4704,14 @@ GlobOpt::ValueNumberDst(IR::Instr **pInstr, Value *src1Val, Value *src2Val)
47044704
case Js::OpCode::Coerce_Str:
47054705
AssertMsg(instr->GetDst()->GetValueType().IsString(),
47064706
"Creator of this instruction should have set the type");
4707+
4708+
// Due to fall through and the fact that Ld_A only takes one source,
4709+
// free the other source here.
4710+
if (instr->GetSrc2() && !(this->IsLoopPrePass() || src1ValueInfo == nullptr || !src1ValueInfo->IsString()))
4711+
{
4712+
instr->FreeSrc2();
4713+
}
4714+
47074715
// fall-through
47084716
case Js::OpCode::Coerce_StrOrRegex:
47094717
// We don't set the ValueType of src1 for Coerce_StrOrRegex, hence skip the ASSERT

‎lib/Backend/IR.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1047,6 +1047,13 @@ bool IR::Instr::IsStElemVariant() const
10471047
this->m_opcode == Js::OpCode::StElemC;
10481048
}
10491049

1050+
bool IR::Instr::DontHoistBailOnNoProfileAboveInGeneratorFunction() const
1051+
{
1052+
return this->m_opcode == Js::OpCode::ResumeYield ||
1053+
this->m_opcode == Js::OpCode::ResumeYieldStar ||
1054+
this->m_opcode == Js::OpCode::GeneratorCreateInterpreterStackFrame;
1055+
}
1056+
10501057
bool IR::Instr::CanChangeFieldValueWithoutImplicitCall() const
10511058
{
10521059
// TODO: Why is InitFld necessary?

‎lib/Backend/IR.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,8 @@ class Instr
336336
static Instr* FindSingleDefInstr(Js::OpCode opCode, Opnd* src);
337337
bool CanAggregateByteCodeUsesAcrossInstr(IR::Instr * instr);
338338

339+
bool DontHoistBailOnNoProfileAboveInGeneratorFunction() const;
340+
339341
// LazyBailOut
340342
bool AreAllOpndsTypeSpecialized() const;
341343
bool IsStFldVariant() const;
@@ -798,6 +800,7 @@ class LabelInstr : public Instr
798800
inline void SetRegion(Region *);
799801
inline Region * GetRegion(void) const;
800802
inline BOOL IsUnreferenced(void) const;
803+
inline BOOL IsGeneratorEpilogueLabel(void) const;
801804

802805
LabelInstr * CloneLabel(BOOL fCreate);
803806

‎lib/Backend/IR.inl

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ Instr::EndsBasicBlock() const
256256
return
257257
this->IsBranchInstr() ||
258258
this->IsExitInstr() ||
259+
this->m_opcode == Js::OpCode::Yield ||
259260
this->m_opcode == Js::OpCode::Ret ||
260261
this->m_opcode == Js::OpCode::Throw ||
261262
this->m_opcode == Js::OpCode::RuntimeTypeError ||
@@ -728,6 +729,13 @@ LabelInstr::IsUnreferenced(void) const
728729
return labelRefs.Empty() && !m_hasNonBranchRef;
729730
}
730731

732+
inline BOOL
733+
LabelInstr::IsGeneratorEpilogueLabel(void) const
734+
{
735+
return this->m_opcode == Js::OpCode::GeneratorEpilogueNoFrameNullOut ||
736+
this->m_opcode == Js::OpCode::GeneratorEpilogueFrameNullOut;
737+
}
738+
731739
inline void
732740
LabelInstr::SetRegion(Region * region)
733741
{

0 commit comments

Comments
 (0)