Skip to content

Commit 84fc1aa

Browse files
rajatdrajatd
committed
[MERGE chakra-core#869] Fixing bug with FromVar hoisting
Merge pull request chakra-core#869 from rajatd:floatTypeSpec 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.
2 parents b69aeb6 + f13ee1a commit 84fc1aa

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

1849318559
if (dstVal == NULL)
@@ -18899,7 +18965,8 @@ GlobOpt::TryHoistInvariant(
1889918965
Value *src2Val,
1890018966
bool isNotTypeSpecConv,
1890118967
const bool lossy,
18902-
const bool forceInvariantHoisting)
18968+
const bool forceInvariantHoisting,
18969+
IR::BailOutKind bailoutKind)
1890318970
{
1890418971
Assert(!this->IsLoopPrePass());
1890518972

@@ -18944,7 +19011,7 @@ GlobOpt::TryHoistInvariant(
1894419011
Assert(tempByteCodeUse->Count() == 0 && propertySymUse == NULL);
1894519012
}
1894619013
#endif
18947-
OptHoistInvariant(instr, block, loop, dstVal, src1Val, isNotTypeSpecConv, lossy);
19014+
OptHoistInvariant(instr, block, loop, dstVal, src1Val, isNotTypeSpecConv, lossy, bailoutKind);
1894819015
return true;
1894919016
}
1895019017

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