Skip to content

Commit 9ebad6a

Browse files
rajatdrajatd
committed
Disable FieldPRE when a type check bailout from a hoisted instruction triggers a rejit
1 parent 26c2c65 commit 9ebad6a

File tree

10 files changed

+60
-18
lines changed

10 files changed

+60
-18
lines changed

‎lib/Backend/BailOut.cpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1212,7 +1212,7 @@ BailOutRecord::BailOutFromLoopBodyHelper(Js::JavascriptCallStackLayout * layout,
12121212

12131213
void BailOutRecord::UpdatePolymorphicFieldAccess(Js::JavascriptFunction * function, BailOutRecord const * bailOutRecord)
12141214
{
1215-
Js::FunctionBody * executeFunction = bailOutRecord->type == Shared ? ((SharedBailOutRecord*)bailOutRecord)->functionBody : function->GetFunctionBody();
1215+
Js::FunctionBody * executeFunction = bailOutRecord->IsShared() ? ((SharedBailOutRecord*)bailOutRecord)->functionBody : function->GetFunctionBody();
12161216
Js::DynamicProfileInfo *dynamicProfileInfo = nullptr;
12171217
if (executeFunction->HasDynamicProfileInfo())
12181218
{
@@ -1708,11 +1708,10 @@ void BailOutRecord::ScheduleFunctionCodeGen(Js::ScriptFunction * function, Js::S
17081708

17091709
Assert(bailOutKind != IR::BailOutInvalid);
17101710

1711-
if ((executeFunction->HasDynamicProfileInfo() && callsCount == 0) ||
1711+
Js::DynamicProfileInfo * profileInfo = executeFunction->HasDynamicProfileInfo() ? executeFunction->GetAnyDynamicProfileInfo() : nullptr;
1712+
if ((profileInfo && callsCount == 0) ||
17121713
PHASE_FORCE(Js::ReJITPhase, executeFunction))
17131714
{
1714-
Js::DynamicProfileInfo * profileInfo = executeFunction->GetAnyDynamicProfileInfo();
1715-
17161715
if ((bailOutKind & (IR::BailOutOnResultConditions | IR::BailOutOnDivSrcConditions)) || bailOutKind == IR::BailOutIntOnly || bailOutKind == IR::BailOnIntMin || bailOutKind == IR::BailOnDivResultNotInt)
17171716
{
17181717
// Note WRT BailOnIntMin: it wouldn't make sense to re-jit without changing anything here, as interpreter will not change the (int) type,
@@ -2155,7 +2154,6 @@ void BailOutRecord::ScheduleFunctionCodeGen(Js::ScriptFunction * function, Js::S
21552154

21562155
if (!reThunk && rejitReason != RejitReason::None)
21572156
{
2158-
Js::DynamicProfileInfo * profileInfo = executeFunction->GetAnyDynamicProfileInfo();
21592157
// REVIEW: Temporary fix for RS1. Disable Rejiting if it looks like it is not fixing the problem.
21602158
// For RS2, turn the rejitCount check into an assert and let's fix all these issues.
21612159
if (profileInfo->GetRejitCount() >= 100 ||
@@ -2223,6 +2221,14 @@ void BailOutRecord::ScheduleFunctionCodeGen(Js::ScriptFunction * function, Js::S
22232221
}
22242222
else if (rejitReason != RejitReason::None)
22252223
{
2224+
if (bailOutRecord->IsForLoopTop() && IR::IsTypeCheckBailOutKind(bailOutRecord->bailOutKind))
2225+
{
2226+
// Disable FieldPRE if we're triggering a type check rejit due to a bailout at the loop top.
2227+
// Most likely this was caused by a CheckFixedFld that was hoisted from a branch block where
2228+
// only certain types flowed, to the loop top, where more types (different or non-equivalent)
2229+
// were flowing in.
2230+
profileInfo->DisableFieldPRE();
2231+
}
22262232
#ifdef REJIT_STATS
22272233
executeFunction->GetScriptContext()->LogRejit(executeFunction, rejitReason);
22282234
#endif

‎lib/Backend/BailOut.h

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ class BailOutInfo
2727
BailOutInfo(uint32 bailOutOffset, Func* bailOutFunc) :
2828
bailOutOffset(bailOutOffset), bailOutFunc(bailOutFunc),
2929
byteCodeUpwardExposedUsed(nullptr), polymorphicCacheIndex((uint)-1), startCallCount(0), startCallInfo(nullptr), bailOutInstr(nullptr),
30-
totalOutParamCount(0), argOutSyms(nullptr), bailOutRecord(nullptr), wasCloned(false), isInvertedBranch(false), sharedBailOutKind(true), outParamInlinedArgSlot(nullptr),
31-
liveVarSyms(nullptr), liveLosslessInt32Syms(nullptr), liveFloat64Syms(nullptr),
30+
totalOutParamCount(0), argOutSyms(nullptr), bailOutRecord(nullptr), wasCloned(false), isInvertedBranch(false), sharedBailOutKind(true), isLoopTopBailOutInfo(false),
31+
outParamInlinedArgSlot(nullptr), liveVarSyms(nullptr), liveLosslessInt32Syms(nullptr), liveFloat64Syms(nullptr),
3232
branchConditionOpnd(nullptr),
3333
stackLiteralBailOutInfoCount(0), stackLiteralBailOutInfo(nullptr)
3434
{
@@ -70,6 +70,7 @@ class BailOutInfo
7070
bool wasCloned;
7171
bool isInvertedBranch;
7272
bool sharedBailOutKind;
73+
bool isLoopTopBailOutInfo;
7374

7475
#if DBG
7576
bool wasCopied;
@@ -201,9 +202,13 @@ class BailOutRecord
201202
{
202203
Normal = 0,
203204
Branch = 1,
204-
Shared = 2
205+
Shared = 2,
206+
SharedForLoopTop = 3
205207
};
206208
BailoutRecordType GetType() { return type; }
209+
void SetType(BailoutRecordType type) { this->type = type; }
210+
bool IsShared() const { return type == Shared || type == SharedForLoopTop; }
211+
bool IsForLoopTop() const { return type == SharedForLoopTop; }
207212
protected:
208213
struct BailOutReturnValue
209214
{

‎lib/Backend/FlowGraph.cpp

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3345,6 +3345,19 @@ BasicBlock::IsLandingPad()
33453345
return nextBlock && nextBlock->loop && nextBlock->isLoopHeader && nextBlock->loop->landingPad == this;
33463346
}
33473347

3348+
BailOutInfo *
3349+
BasicBlock::CreateLoopTopBailOutInfo(GlobOpt * globOpt)
3350+
{
3351+
IR::Instr * firstInstr = this->GetFirstInstr();
3352+
BailOutInfo* bailOutInfo = JitAnew(globOpt->func->m_alloc, BailOutInfo, firstInstr->GetByteCodeOffset(), firstInstr->m_func);
3353+
bailOutInfo->isLoopTopBailOutInfo = true;
3354+
globOpt->FillBailOutInfo(this, bailOutInfo);
3355+
#if ENABLE_DEBUG_CONFIG_OPTIONS
3356+
bailOutInfo->bailOutOpcode = Js::OpCode::LoopBodyStart;
3357+
#endif
3358+
return bailOutInfo;
3359+
}
3360+
33483361
IR::Instr *
33493362
FlowGraph::RemoveInstr(IR::Instr *instr, GlobOpt * globOpt)
33503363
{
@@ -4539,13 +4552,7 @@ BasicBlock::MergePredBlocksValueMaps(GlobOpt* globOpt)
45394552
{
45404553
// Capture bail out info in case we have optimization that needs it
45414554
Assert(this->loop->bailOutInfo == nullptr);
4542-
IR::Instr * firstInstr = this->GetFirstInstr();
4543-
this->loop->bailOutInfo = JitAnew(globOpt->func->m_alloc, BailOutInfo,
4544-
firstInstr->GetByteCodeOffset(), firstInstr->m_func);
4545-
globOpt->FillBailOutInfo(this, this->loop->bailOutInfo);
4546-
#if ENABLE_DEBUG_CONFIG_OPTIONS
4547-
this->loop->bailOutInfo->bailOutOpcode = Js::OpCode::LoopBodyStart;
4548-
#endif
4555+
this->loop->bailOutInfo = this->CreateLoopTopBailOutInfo(globOpt);
45494556
}
45504557

45514558
// If loop pre-pass, don't insert convert from type-spec to var

‎lib/Backend/FlowGraph.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,7 @@ class BasicBlock
346346
}
347347

348348
bool IsLandingPad();
349+
BailOutInfo * CreateLoopTopBailOutInfo(GlobOpt * globOpt);
349350

350351
// GlobOpt Stuff
351352
public:

‎lib/Backend/GlobOptFields.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ GlobOpt::DoFieldPRE(Loop *loop) const
117117
return true;
118118
}
119119

120+
if (this->func->HasProfileInfo() && this->func->GetReadOnlyProfileInfo()->IsFieldPREDisabled())
121+
{
122+
return false;
123+
}
124+
120125
return DoFieldOpts(loop);
121126
}
122127

‎lib/Backend/JITTimeProfileInfo.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ JITTimeProfileInfo::InitializeJITProfileData(
147147
data->flags |= profileInfo->IsPowIntIntTypeSpecDisabled() ? Flags_disablePowIntIntTypeSpec : 0;
148148
data->flags |= profileInfo->IsTagCheckDisabled() ? Flags_disableTagCheck : 0;
149149
data->flags |= profileInfo->IsOptimizeTryFinallyDisabled() ? Flags_disableOptimizeTryFinally : 0;
150+
data->flags |= profileInfo->IsFieldPREDisabled() ? Flags_disableFieldPRE : 0;
150151
}
151152

152153
const Js::LdLenInfo *
@@ -509,6 +510,12 @@ JITTimeProfileInfo::IsOptimizeTryFinallyDisabled() const
509510
return TestFlag(Flags_disableOptimizeTryFinally);
510511
}
511512

513+
bool
514+
JITTimeProfileInfo::IsFieldPREDisabled() const
515+
{
516+
return TestFlag(Flags_disableFieldPRE);
517+
}
518+
512519
bool
513520
JITTimeProfileInfo::HasLdFldCallSiteInfo() const
514521
{

‎lib/Backend/JITTimeProfileInfo.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ class JITTimeProfileInfo
6767
bool IsPowIntIntTypeSpecDisabled() const;
6868
bool IsTagCheckDisabled() const;
6969
bool IsOptimizeTryFinallyDisabled() const;
70+
bool IsFieldPREDisabled() const;
7071

7172
private:
7273
enum ProfileDataFlags : int64
@@ -108,7 +109,8 @@ class JITTimeProfileInfo
108109
Flags_disableLoopImplicitCallInfo = 1ll << 33,
109110
Flags_disablePowIntIntTypeSpec = 1ll << 34,
110111
Flags_disableTagCheck = 1ll << 35,
111-
Flags_disableOptimizeTryFinally = 1ll << 36
112+
Flags_disableOptimizeTryFinally = 1ll << 36,
113+
Flags_disableFieldPRE = 1ll << 37
112114
};
113115

114116
Js::ProfileId GetProfiledArrayCallSiteCount() const;

‎lib/Backend/Lower.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13495,7 +13495,7 @@ Lowerer::GenerateBailOut(IR::Instr * instr, IR::BranchInstr * branchInstr, IR::L
1349513495
indexOpnd, IR::IntConstOpnd::New(bailOutInfo->polymorphicCacheIndex, TyUint32, this->m_func), instr, false);
1349613496
}
1349713497

13498-
if (bailOutInfo->bailOutRecord->GetType() == BailOutRecord::BailoutRecordType::Shared)
13498+
if (bailOutInfo->bailOutRecord->IsShared())
1349913499
{
1350013500
IR::Opnd *functionBodyOpnd;
1350113501
if (this->m_func->IsOOPJIT())
@@ -13629,6 +13629,10 @@ Lowerer::GenerateBailOut(IR::Instr * instr, IR::BranchInstr * branchInstr, IR::L
1362913629
{
1363013630
bailOutRecord = NativeCodeDataNewZ(this->m_func->GetNativeCodeDataAllocator(),
1363113631
SharedBailOutRecord, bailOutInfo->bailOutOffset, bailOutInfo->polymorphicCacheIndex, instr->GetBailOutKind(), bailOutInfo->bailOutFunc);
13632+
if (bailOutInfo->isLoopTopBailOutInfo)
13633+
{
13634+
bailOutRecord->SetType(BailOutRecord::BailoutRecordType::SharedForLoopTop);
13635+
}
1363213636
}
1363313637
else
1363413638
{

‎lib/Runtime/Language/DynamicProfileInfo.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1735,6 +1735,7 @@ namespace Js
17351735
_u(" disableStackArgOpt : %s\n")
17361736
_u(" disableTagCheck : %s\n")
17371737
_u(" disableOptimizeTryFinally : %s\n"),
1738+
_u(" disableFieldPRE : %s\n"),
17381739
IsTrueOrFalse(this->bits.disableAggressiveIntTypeSpec),
17391740
IsTrueOrFalse(this->bits.disableAggressiveIntTypeSpec_jitLoopBody),
17401741
IsTrueOrFalse(this->bits.disableAggressiveMulIntTypeSpec),
@@ -1771,7 +1772,8 @@ namespace Js
17711772
IsTrueOrFalse(this->bits.disablePowIntIntTypeSpec),
17721773
IsTrueOrFalse(this->bits.disableStackArgOpt),
17731774
IsTrueOrFalse(this->bits.disableTagCheck),
1774-
IsTrueOrFalse(this->bits.disableOptimizeTryFinally));
1775+
IsTrueOrFalse(this->bits.disableOptimizeTryFinally),
1776+
IsTrueOrFalse(this->bits.disableFieldPRE));
17751777
}
17761778
}
17771779

‎lib/Runtime/Language/DynamicProfileInfo.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,7 @@ namespace Js
568568
Field(bool) disableStackArgOpt : 1;
569569
Field(bool) disableTagCheck : 1;
570570
Field(bool) disableOptimizeTryFinally : 1;
571+
Field(bool) disableFieldPRE : 1;
571572
};
572573
Field(Bits) bits;
573574

@@ -868,6 +869,8 @@ namespace Js
868869
void DisableTagCheck() { this->bits.disableTagCheck = true; }
869870
bool IsOptimizeTryFinallyDisabled() const { return bits.disableOptimizeTryFinally; }
870871
void DisableOptimizeTryFinally() { this->bits.disableOptimizeTryFinally = true; }
872+
bool IsFieldPREDisabled() const { return bits.disableFieldPRE; }
873+
void DisableFieldPRE() { this->bits.disableFieldPRE = true; }
871874

872875
static bool IsCallSiteNoInfo(Js::LocalFunctionId functionId) { return functionId == CallSiteNoInfo; }
873876
int IncRejitCount() { return this->rejitCount++; }

0 commit comments

Comments
 (0)