Skip to content

Commit 5aed68b

Browse files
rajatdrajatd
committed
[MERGE chakra-core#4366 @rajatd] Make Globopt fold branches on same operands instead of lowerer
Merge pull request chakra-core#4366 from rajatd:typeofLoopTail In case when the two sources of a branch were the result of the identical typeof operations, globopt CSE'd the typeofs making both the sources of the branch to be the same operands. The lowerer then tries to do the smart thing by detecting identical sources and either emitting a direct branch or removing the branch altogether (depedending on the branch instruction). This 'optimizing' behaviour of the lowerer becomes a problem when the target of the branch being removed is a loop top because the register allocator expects at least one back edge to a loop top label. I'm fixing this by doing slightly better than what we do today - I'm adding code to the globopt to detect identical values on the operands of a branch and optimize it if possible. Globopt does the right thing with respect to loop top labels by editing the flowgraph to remove dead blocks.
2 parents 1ac2d65 + d4556ba commit 5aed68b

5 files changed

Lines changed: 55 additions & 13 deletions

File tree

‎lib/Backend/GlobOpt.cpp‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6593,6 +6593,13 @@ GlobOpt::OptConstFoldBranch(IR::Instr *instr, Value *src1Val, Value*src2Val, Val
65936593
src2Var = this->GetConstantVar(instr->GetSrc2(), src2Val);
65946594
}
65956595

6596+
auto AreSourcesEqual = [&](Value * val1, Value * val2) -> bool
6597+
{
6598+
// NaN !== NaN, and objects can have valueOf/toString
6599+
return val1->IsEqualTo(val2) &&
6600+
val1->GetValueInfo()->IsPrimitive() && !val1->GetValueInfo()->IsFloat();
6601+
};
6602+
65966603
// Make sure GetConstantVar only returns primitives.
65976604
// TODO: OOP JIT, enabled these asserts
65986605
//Assert(!src1Var || !Js::JavascriptOperators::IsObject(src1Var));
@@ -6609,6 +6616,10 @@ GlobOpt::OptConstFoldBranch(IR::Instr *instr, Value *src1Val, Value*src2Val, Val
66096616
{ \
66106617
result = (TYPE)left64 CMP (TYPE)right64; \
66116618
} \
6619+
else if (AreSourcesEqual(src1Val, src2Val)) \
6620+
{ \
6621+
result = 0 CMP 0; \
6622+
} \
66126623
else \
66136624
{ \
66146625
return false; \
@@ -6631,7 +6642,11 @@ GlobOpt::OptConstFoldBranch(IR::Instr *instr, Value *src1Val, Value*src2Val, Val
66316642
{
66326643
if (BoolAndIntStaticAndTypeMismatch(src1Val, src2Val, src1Var, src2Var))
66336644
{
6634-
result = false;
6645+
result = false;
6646+
}
6647+
else if (AreSourcesEqual(src1Val, src2Val))
6648+
{
6649+
result = true;
66356650
}
66366651
else
66376652
{
@@ -6656,6 +6671,10 @@ GlobOpt::OptConstFoldBranch(IR::Instr *instr, Value *src1Val, Value*src2Val, Val
66566671
{
66576672
result = true;
66586673
}
6674+
else if (AreSourcesEqual(src1Val, src2Val))
6675+
{
6676+
result = false;
6677+
}
66596678
else
66606679
{
66616680
return false;
@@ -6693,6 +6712,10 @@ GlobOpt::OptConstFoldBranch(IR::Instr *instr, Value *src1Val, Value*src2Val, Val
66936712
{
66946713
result = false;
66956714
}
6715+
else if (AreSourcesEqual(src1Val, src2Val))
6716+
{
6717+
result = true;
6718+
}
66966719
else
66976720
{
66986721
return false;
@@ -6731,6 +6754,10 @@ GlobOpt::OptConstFoldBranch(IR::Instr *instr, Value *src1Val, Value*src2Val, Val
67316754
{
67326755
result = true;
67336756
}
6757+
else if (AreSourcesEqual(src1Val, src2Val))
6758+
{
6759+
result = false;
6760+
}
67346761
else
67356762
{
67366763
return false;

‎lib/Backend/Lower.cpp‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22372,17 +22372,7 @@ Lowerer::TryGenerateFastBrOrCmTypeOf(IR::Instr *instr, IR::Instr **prev, bool is
2237222372
*pfNoLower = true;
2237322373
if (instr->IsBranchInstr())
2237422374
{
22375-
if (instrSrc1->IsEqual(instrSrc2))
22376-
{
22377-
if (!isNeqOp)
22378-
{
22379-
InsertBranch(Js::OpCode::Br, instr->AsBranchInstr()->GetTarget(), instr);
22380-
}
22381-
}
22382-
else
22383-
{
22384-
InsertCompareBranch(instrSrc1, instrSrc2, isNeqOp ? Js::OpCode::BrNeq_A : Js::OpCode::BrEq_A, instr->AsBranchInstr()->GetTarget(), instr);
22385-
}
22375+
InsertCompareBranch(instrSrc1, instrSrc2, isNeqOp ? Js::OpCode::BrNeq_A : Js::OpCode::BrEq_A, instr->AsBranchInstr()->GetTarget(), instr);
2238622376
instr->Remove();
2238722377
}
2238822378
else

‎lib/Backend/ValueInfo.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ class Value
315315
void SetValueInfo(ValueInfo * newValueInfo) { Assert(newValueInfo); this->valueInfo = newValueInfo; }
316316

317317
Value * Copy(JitArenaAllocator * allocator, ValueNumber newValueNumber) const { return Value::New(allocator, newValueNumber, this->ShareValueInfo()); }
318-
318+
bool IsEqualTo(Value * other) { return this->valueNumber == other->valueNumber; }
319319
#if DBG_DUMP
320320
_NOINLINE void Dump() const { Output::Print(_u("0x%X ValueNumber: %3d, -> "), this, this->valueNumber); this->valueInfo->Dump(); }
321321
#endif

‎test/Optimizer/bug14661401.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
function test0() {
7+
var loopInvariant = 44;
8+
var obj0 = {};
9+
10+
while (typeof obj0.prop1) {
11+
var __loopvar1 = loopInvariant, __loopSecondaryVar1_1 = loopInvariant - 14;
12+
do {
13+
__loopvar1 += 4;
14+
} while (typeof obj0.prop1 !== typeof obj0.prop1);
15+
break;
16+
}
17+
}
18+
test0();
19+
test0();
20+
print("passed");

‎test/Optimizer/rlexe.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1429,4 +1429,9 @@
14291429
<files>invalidIVRangeBug.js</files>
14301430
</default>
14311431
</test>
1432+
<test>
1433+
<default>
1434+
<files>bug14661401.js</files>
1435+
</default>
1436+
</test>
14321437
</regress-exe>

0 commit comments

Comments
 (0)