Skip to content

Commit f13ee1a

Browse files
rajatdrajatd
committed
Fixing bug with FromVar hoisting
A combination of math type spec, copy-prop, nested loops and invariant hoisting is required to repro this bug. We end up hoisting a FromVar from a region where it doesn't need a bailout to one where it does. We should then use the value type of the src in the destination region to put the right bailout on FromVar. Addressed comments Added lossy type checks Added an assert for the expected bailouts on FromVar
1 parent 7975a07 commit f13ee1a

3 files changed

Lines changed: 156 additions & 65 deletions

File tree

‎lib/Backend/GlobOpt.cpp‎

Lines changed: 130 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -13734,57 +13734,60 @@ GlobOpt::ToTypeSpecUse(IR::Instr *instr, IR::Opnd *opnd, BasicBlock *block, Valu
1373413734
Assert(loop->bailOutInfo);
1373513735
}
1373613736

13737-
if(toType == TyInt32 && opcode == Js::OpCode::FromVar)
13737+
if (opcode == Js::OpCode::FromVar)
1373813738
{
13739-
Assert(valueInfo);
13740-
if(lossy)
13739+
13740+
if (toType == TyInt32)
1374113741
{
13742-
if(!valueInfo->IsPrimitive() && !IsTypeSpecialized(varSym, block))
13742+
Assert(valueInfo);
13743+
if (lossy)
1374313744
{
13744-
// Lossy conversions to int32 on non-primitive values may have implicit calls to toString or valueOf, which
13745-
// may be overridden to have a side effect. The side effect needs to happen every time the conversion is
13746-
// supposed to happen, so the resulting lossy int32 value cannot be reused. Bail out on implicit calls.
13747-
Assert(DoLossyIntTypeSpec());
13745+
if (!valueInfo->IsPrimitive() && !IsTypeSpecialized(varSym, block))
13746+
{
13747+
// Lossy conversions to int32 on non-primitive values may have implicit calls to toString or valueOf, which
13748+
// may be overridden to have a side effect. The side effect needs to happen every time the conversion is
13749+
// supposed to happen, so the resulting lossy int32 value cannot be reused. Bail out on implicit calls.
13750+
Assert(DoLossyIntTypeSpec());
13751+
13752+
bailOutKind = IR::BailOutOnNotPrimitive;
13753+
isBailout = true;
13754+
}
13755+
}
13756+
else if (!valueInfo->IsInt())
13757+
{
13758+
// The operand is likely an int (hence the request to convert to int), so bail out if it's not an int. Only
13759+
// bail out if a lossless conversion to int is requested. Lossy conversions to int such as in (a | 0) don't
13760+
// need to bail out.
13761+
if (bailOutKind == IR::BailOutExpectingInteger)
13762+
{
13763+
Assert(IsSwitchOptEnabled());
13764+
}
13765+
else
13766+
{
13767+
Assert(DoAggressiveIntTypeSpec());
13768+
}
1374813769

13749-
bailOutKind = IR::BailOutOnNotPrimitive;
1375013770
isBailout = true;
1375113771
}
1375213772
}
13753-
else if(!valueInfo->IsInt())
13773+
else if (toType == TyFloat64 &&
13774+
(!valueInfo || !valueInfo->IsNumber()))
1375413775
{
13755-
// The operand is likely an int (hence the request to convert to int), so bail out if it's not an int. Only
13756-
// bail out if a lossless conversion to int is requested. Lossy conversions to int such as in (a | 0) don't
13757-
// need to bail out.
13758-
if(bailOutKind == IR::BailOutExpectingInteger)
13759-
{
13760-
Assert(IsSwitchOptEnabled());
13761-
}
13762-
else
13763-
{
13764-
Assert(DoAggressiveIntTypeSpec());
13765-
}
13766-
13776+
// Bailout if converting vars to float if we can't prove they are floats:
13777+
// x = str + float; -> need to bailout if str is a string
13778+
//
13779+
// x = obj * 0.1;
13780+
// y = obj * 0.2; -> if obj has valueof, we'll only call valueof once on the FromVar conversion...
13781+
Assert(bailOutKind != IR::BailOutInvalid);
1376713782
isBailout = true;
1376813783
}
13769-
}
13770-
else if (toType == TyFloat64 && opcode == Js::OpCode::FromVar
13771-
&& (!valueInfo || !valueInfo->IsNumber()))
13772-
{
13773-
// Bailout if converting vars to float if we can't prove they are floats:
13774-
// x = str + float; -> need to bailout if str is a string
13775-
//
13776-
// x = obj * 0.1;
13777-
// y = obj * 0.2; -> if obj has valueof, we'll only call valueof once on the FromVar conversion...
13778-
Assert(bailOutKind != IR::BailOutInvalid);
13779-
isBailout = true;
13780-
}
13781-
else if (IRType_IsSimd128(toType)
13782-
&& opcode == Js::OpCode::FromVar
13783-
&& (!valueInfo || !valueInfo->IsSimd128(toType)))
13784-
{
13784+
else if (IRType_IsSimd128(toType) &&
13785+
(!valueInfo || !valueInfo->IsSimd128(toType)))
13786+
{
1378513787
Assert(toType == TySimd128F4 && bailOutKind == IR::BailOutSimd128F4Only
1378613788
|| toType == TySimd128I4 && bailOutKind == IR::BailOutSimd128I4Only);
1378713789
isBailout = true;
13790+
}
1378813791
}
1378913792

1379013793
if (isBailout)
@@ -13894,7 +13897,7 @@ GlobOpt::ToTypeSpecUse(IR::Instr *instr, IR::Opnd *opnd, BasicBlock *block, Valu
1389413897
if (block->loop)
1389513898
{
1389613899
Assert(!this->IsLoopPrePass());
13897-
isHoisted = this->TryHoistInvariant(newInstr, block, val, val, nullptr, false, lossy);
13900+
isHoisted = this->TryHoistInvariant(newInstr, block, val, val, nullptr, false, lossy, false, bailOutKind);
1389813901
}
1389913902

1390013903
if (isBailout)
@@ -18456,34 +18459,97 @@ GlobOpt::OptHoistInvariant(
1845618459
Value *dstVal,
1845718460
Value *const src1Val,
1845818461
bool isNotTypeSpecConv,
18459-
bool lossy)
18462+
bool lossy,
18463+
IR::BailOutKind bailoutKind)
1846018464
{
1846118465
BasicBlock *landingPad = loop->landingPad;
1846218466
IR::RegOpnd *dst = instr->GetDst() ? instr->GetDst()->AsRegOpnd() : nullptr;
1846318467
if(dst)
1846418468
{
18465-
switch(instr->m_opcode)
18466-
{
18467-
case Js::OpCode::CmEq_I4:
18468-
case Js::OpCode::CmNeq_I4:
18469-
case Js::OpCode::CmLt_I4:
18470-
case Js::OpCode::CmLe_I4:
18471-
case Js::OpCode::CmGt_I4:
18472-
case Js::OpCode::CmGe_I4:
18473-
case Js::OpCode::CmUnLt_I4:
18474-
case Js::OpCode::CmUnLe_I4:
18475-
case Js::OpCode::CmUnGt_I4:
18476-
case Js::OpCode::CmUnGe_I4:
18477-
// These operations are a special case. They generate a lossy int value, and the var sym is initialized using
18478-
// Conv_Bool. A sym cannot be live only as a lossy int sym, the var needs to be live as well since the lossy int
18479-
// sym cannot be used to convert to var. We don't know however, whether the Conv_Bool will be hoisted. The idea
18480-
// currently is that the sym is only used on the path in which it is initialized inside the loop. So, don't
18481-
// hoist any liveness info for the dst.
18482-
if (!this->GetIsAsmJSFunc())
18483-
{
18484-
lossy = true;
18469+
switch (instr->m_opcode)
18470+
{
18471+
case Js::OpCode::CmEq_I4:
18472+
case Js::OpCode::CmNeq_I4:
18473+
case Js::OpCode::CmLt_I4:
18474+
case Js::OpCode::CmLe_I4:
18475+
case Js::OpCode::CmGt_I4:
18476+
case Js::OpCode::CmGe_I4:
18477+
case Js::OpCode::CmUnLt_I4:
18478+
case Js::OpCode::CmUnLe_I4:
18479+
case Js::OpCode::CmUnGt_I4:
18480+
case Js::OpCode::CmUnGe_I4:
18481+
// These operations are a special case. They generate a lossy int value, and the var sym is initialized using
18482+
// Conv_Bool. A sym cannot be live only as a lossy int sym, the var needs to be live as well since the lossy int
18483+
// sym cannot be used to convert to var. We don't know however, whether the Conv_Bool will be hoisted. The idea
18484+
// currently is that the sym is only used on the path in which it is initialized inside the loop. So, don't
18485+
// hoist any liveness info for the dst.
18486+
if (!this->GetIsAsmJSFunc())
18487+
{
18488+
lossy = true;
18489+
}
18490+
break;
18491+
18492+
case Js::OpCode::FromVar:
18493+
{
18494+
StackSym* src1StackSym = IR::RegOpnd::TryGetStackSym(instr->GetSrc1());
18495+
18496+
if (instr->HasBailOutInfo())
18497+
{
18498+
IR::BailOutKind bailoutKind = instr->GetBailOutKind();
18499+
Assert(bailoutKind == IR::BailOutIntOnly ||
18500+
bailoutKind == IR::BailOutExpectingInteger ||
18501+
bailoutKind == IR::BailOutOnNotPrimitive ||
18502+
bailoutKind == IR::BailOutNumberOnly ||
18503+
bailoutKind == IR::BailOutPrimitiveButString ||
18504+
bailoutKind == IR::BailOutSimd128F4Only ||
18505+
bailoutKind == IR::BailOutSimd128I4Only);
18506+
}
18507+
else if (src1StackSym && bailoutKind != IR::BailOutInvalid)
18508+
{
18509+
// We may be hoisting FromVar from a region where it didn't need a bailout (src1 had a definite value type) to a region
18510+
// where it would. In such cases, the FromVar needs a bailout based on the value type of src1 in its new position.
18511+
Assert(!src1StackSym->IsTypeSpec());
18512+
Value* landingPadSrc1val = FindValue(landingPad->globOptData.symToValueMap, src1StackSym);
18513+
Assert(src1Val->GetValueNumber() == landingPadSrc1val->GetValueNumber());
18514+
18515+
ValueInfo *src1ValueInfo = src1Val->GetValueInfo();
18516+
ValueInfo *landingPadSrc1ValueInfo = landingPadSrc1val->GetValueInfo();
18517+
IRType dstType = dst->GetType();
18518+
18519+
const auto AddBailOutToFromVar = [&]()
18520+
{
18521+
instr->GetSrc1()->SetValueType(landingPadSrc1val->GetValueInfo()->Type());
18522+
EnsureBailTarget(loop);
18523+
instr = instr->ConvertToBailOutInstr(instr, bailoutKind);
18524+
};
18525+
18526+
// A definite type in the source position and not a definite type in the destination (landing pad)
18527+
// and no bailout on the instruction; we should put a bailout on the hoisted instruction.
18528+
if (dstType == TyInt32)
18529+
{
18530+
if (lossy)
18531+
{
18532+
if ((src1ValueInfo->IsPrimitive() || IsTypeSpecialized(src1StackSym, block)) && // didn't need a lossy type spec bailout in the source block
18533+
(!landingPadSrc1ValueInfo->IsPrimitive() && !IsTypeSpecialized(src1StackSym, landingPad))) // needs a lossy type spec bailout in the landing pad
18534+
{
18535+
bailoutKind = IR::BailOutOnNotPrimitive;
18536+
AddBailOutToFromVar();
18537+
}
18538+
}
18539+
else if (src1ValueInfo->IsInt() && !landingPadSrc1ValueInfo->IsInt())
18540+
{
18541+
AddBailOutToFromVar();
18542+
}
1848518543
}
18486-
break;
18544+
else if ((dstType == TyFloat64 && src1ValueInfo->IsNumber() && !landingPadSrc1ValueInfo->IsNumber()) ||
18545+
(IRType_IsSimd128(dstType) && src1ValueInfo->IsSimd128() && !landingPadSrc1ValueInfo->IsSimd128()))
18546+
{
18547+
AddBailOutToFromVar();
18548+
}
18549+
}
18550+
18551+
break;
18552+
}
1848718553
}
1848818554

1848918555
if (dstVal == NULL)
@@ -18895,7 +18961,8 @@ GlobOpt::TryHoistInvariant(
1889518961
Value *src2Val,
1889618962
bool isNotTypeSpecConv,
1889718963
const bool lossy,
18898-
const bool forceInvariantHoisting)
18964+
const bool forceInvariantHoisting,
18965+
IR::BailOutKind bailoutKind)
1889918966
{
1890018967
Assert(!this->IsLoopPrePass());
1890118968

@@ -18940,7 +19007,7 @@ GlobOpt::TryHoistInvariant(
1894019007
Assert(tempByteCodeUse->Count() == 0 && propertySymUse == NULL);
1894119008
}
1894219009
#endif
18943-
OptHoistInvariant(instr, block, loop, dstVal, src1Val, isNotTypeSpecConv, lossy);
19010+
OptHoistInvariant(instr, block, loop, dstVal, src1Val, isNotTypeSpecConv, lossy, bailoutKind);
1894419011
return true;
1894519012
}
1894619013

‎lib/Backend/GlobOpt.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1575,8 +1575,9 @@ class GlobOpt
15751575
bool OptIsInvariant(Sym *sym, BasicBlock *block, Loop *loop, Value *srcVal, bool isNotTypeSpecConv, bool allowNonPrimitives, Value **loopHeadValRef = nullptr);
15761576
bool OptDstIsInvariant(IR::RegOpnd *dst);
15771577
bool OptIsInvariant(IR::Instr *instr, BasicBlock *block, Loop *loop, Value *src1Val, Value *src2Val, bool isNotTypeSpecConv, const bool forceInvariantHoisting = false);
1578-
void OptHoistInvariant(IR::Instr *instr, BasicBlock *block, Loop *loop, Value *dstVal, Value *const src1Val, bool isNotTypeSpecConv, bool lossy = false);
1579-
bool TryHoistInvariant(IR::Instr *instr, BasicBlock *block, Value *dstVal, Value *src1Val, Value *src2Val, bool isNotTypeSpecConv, const bool lossy = false, const bool forceInvariantHoisting = false);
1578+
void OptHoistInvariant(IR::Instr *instr, BasicBlock *block, Loop *loop, Value *dstVal, Value *const src1Val, bool isNotTypeSpecConv, bool lossy = false, IR::BailOutKind bailoutKind = IR::BailOutInvalid);
1579+
bool TryHoistInvariant(IR::Instr *instr, BasicBlock *block, Value *dstVal, Value *src1Val, Value *src2Val, bool isNotTypeSpecConv,
1580+
const bool lossy = false, const bool forceInvariantHoisting = false, IR::BailOutKind bailoutKind = IR::BailOutInvalid);
15801581
void HoistInvariantValueInfo(ValueInfo *const invariantValueInfoToHoist, Value *const valueToUpdate, BasicBlock *const targetBlock);
15811582
public:
15821583
static bool IsTypeSpecPhaseOff(Func* func);

‎test/Optimizer/TypeSpec.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,26 @@ function testrem(){
5959
testrem();
6060
testrem();
6161

62+
// VSO bug 5497066
63+
// Hoisting FromVar from a region where it doesn't need a bailout
64+
// to one where it does
65+
function test2() {
66+
var func1 = function (argMath2) {
67+
for (; __loopvar0 != 2 && d < argMath2; ) {
68+
__loopvar0 += 3;
69+
}
70+
};
71+
var i32 = new Int32Array();
72+
var d = -1865727919761880000;
73+
var uniqobj0 = Object();
74+
var __loopvar0 = -1;
75+
76+
var count = 1;
77+
78+
for (; count ? true : func1(uniqobj0); i32[1317940107]) {
79+
count--;
80+
}
81+
uniqobj0;
82+
}
83+
test2();
84+
test2();

0 commit comments

Comments
 (0)