Skip to content

Commit 34967a4

Browse files
author
Meghana Gupta
committed
[MERGE #5204 @meg-gupta] OS#17531342 : Fix path dependent branch folding bug
Merge pull request #5204 from meg-gupta:fixgetterbug
2 parents e953348 + 3dacce3 commit 34967a4

File tree

8 files changed

+153
-49
lines changed

8 files changed

+153
-49
lines changed

‎lib/Backend/FlowGraph.cpp

Lines changed: 18 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4524,9 +4524,7 @@ static bool IsCopyTypeInstr(IR::Instr *instr)
45244524
case Js::OpCode::LdC_A_I4:
45254525
case Js::OpCode::Ld_I4:
45264526
case Js::OpCode::Ld_A:
4527-
case Js::OpCode::StFld:
4528-
case Js::OpCode::LdFld:
4529-
case Js::OpCode::InitFld: return true;
4527+
case Js::OpCode::LdFld: return true;
45304528
default:
45314529
return false;
45324530
}
@@ -4620,7 +4618,8 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46204618
BVSparse<JitArenaAllocator> currentPathDefines(globOpt->alloc);
46214619
IR::Instr *currentInlineeEnd = nullptr, *unskippedInlineeEnd = nullptr;
46224620

4623-
auto UpdateValueForCopyTypeInstr = [&](IR::Instr *instr) {
4621+
auto UpdateValueForCopyTypeInstr = [&](IR::Instr *instr) -> Value* {
4622+
Value * dstValue = nullptr;
46244623
if (instr->m_opcode == Js::OpCode::LdFld)
46254624
{
46264625
// Special handling for LdFld
@@ -4635,43 +4634,44 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46354634
PropertySym *prop = PropertySym::Find(objSym ? objSym->m_id : originalPropertySym->m_stackSym->m_id, originalPropertySym->m_propertyId, globOpt->func);
46364635
if (prop)
46374636
{
4638-
FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetStackSym(), prop);
4637+
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetStackSym(), prop);
46394638
}
46404639
else
46414640
{
46424641
Value ** localDstValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetStackSym());
4643-
*localDstValue = nullptr;
4642+
dstValue = *localDstValue = nullptr;
46444643
}
46454644
}
46464645
}
46474646
else if (instr->GetSrc1()->GetStackSym())
46484647
{
46494648
StackSym* src1Sym = instr->GetSrc1()->GetStackSym();
4650-
FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetSym(), src1Sym);
4649+
dstValue = FindValueInLocalThenGlobalValueTableAndUpdate(globOpt, localSymToValueMap, instr, instr->GetDst()->GetSym(), src1Sym);
46514650
}
46524651
else if (instr->GetSrc1()->IsIntConstOpnd())
46534652
{
46544653
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4655-
*localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsIntConstOpnd()->AsInt32(), instr);
4654+
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsIntConstOpnd()->AsInt32(), instr);
46564655
}
46574656
else if (instr->GetSrc1()->IsInt64ConstOpnd())
46584657
{
46594658
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4660-
*localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsInt64ConstOpnd()->GetValue(), instr);
4659+
dstValue = *localValue = globOpt->GetIntConstantValue(instr->GetSrc1()->AsInt64ConstOpnd()->GetValue(), instr);
46614660
}
46624661
else
46634662
{
46644663
ValueType src1Value = instr->GetSrc1()->GetValueType();
46654664
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
46664665
if (src1Value.IsUndefined() || src1Value.IsBoolean())
46674666
{
4668-
*localValue = globOpt->GetVarConstantValue(instr->GetSrc1()->AsAddrOpnd());
4667+
dstValue = *localValue = globOpt->GetVarConstantValue(instr->GetSrc1()->AsAddrOpnd());
46694668
}
46704669
else
46714670
{
4672-
*localValue = nullptr;
4671+
dstValue = *localValue = nullptr;
46734672
}
46744673
}
4674+
return dstValue;
46754675
};
46764676

46774677
FOREACH_INSTR_IN_BLOCK(instr, this)
@@ -4684,30 +4684,6 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
46844684
{
46854685
unskippedInlineeEnd = currentInlineeEnd = instr;
46864686
}
4687-
else if (instr->GetDst())
4688-
{
4689-
if (instr->GetDst()->GetSym())
4690-
{
4691-
if (IsCopyTypeInstr(instr))
4692-
{
4693-
UpdateValueForCopyTypeInstr(instr);
4694-
}
4695-
else if(instr->m_opcode == Js::OpCode::NewScObjectLiteral)
4696-
{
4697-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4698-
if (instr->GetDst()->GetValueType() == ValueType::UninitializedObject)
4699-
{
4700-
*localValue = globOpt->NewGenericValue(ValueType::UninitializedObject, instr->GetDst());
4701-
}
4702-
}
4703-
else
4704-
{
4705-
// complex instr, can't track value, insert nullptr
4706-
Value **localValue = localSymToValueMap->FindOrInsertNew(instr->GetDst()->GetSym());
4707-
*localValue = nullptr;
4708-
}
4709-
}
4710-
}
47114687
} NEXT_INSTR_IN_BLOCK;
47124688

47134689
IR::Instr * instr = this->GetLastInstr();
@@ -4747,7 +4723,12 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
47474723

47484724
if (IsCopyTypeInstr(instr))
47494725
{
4750-
UpdateValueForCopyTypeInstr(instr);
4726+
Value *dstValue = UpdateValueForCopyTypeInstr(instr);
4727+
if (instr->m_opcode == Js::OpCode::LdFld && !dstValue)
4728+
{
4729+
// We cannot skip a LdFld if we didnt find its valueInfo in the localValueTable
4730+
return;
4731+
}
47514732
}
47524733
else
47534734
{
@@ -4848,6 +4829,7 @@ BasicBlock::CheckLegalityAndFoldPathDepBranches(GlobOpt* globOpt)
48484829
if (currentInlineeEnd != nullptr && currentInlineeEnd != unskippedInlineeEnd)
48494830
{
48504831
this->GetLastInstr()->InsertBefore(currentInlineeEnd->Copy());
4832+
globOpt->ProcessInlineeEnd(currentInlineeEnd);
48514833
currentInlineeEnd = nullptr;
48524834
}
48534835
// We are adding an unconditional branch, go over all the current successors and remove the ones that are dead now

‎lib/Backend/GlobOpt.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,6 @@ GlobOpt::OptBlock(BasicBlock *block)
475475
PrepareLoopArrayCheckHoist();
476476

477477
block->MergePredBlocksValueMaps(this);
478-
block->PathDepBranchFolding(this);
479478

480479
this->intOverflowCurrentlyMattersInRange = true;
481480
this->intOverflowDoesNotMatterRange = this->currentBlock->intOverflowDoesNotMatterRange;
@@ -613,6 +612,8 @@ GlobOpt::OptBlock(BasicBlock *block)
613612
}
614613
}
615614

615+
block->PathDepBranchFolding(this);
616+
616617
#if DBG
617618
// The set of live lossy int32 syms should be a subset of all live int32 syms
618619
this->tempBv->And(block->globOptData.liveInt32Syms, block->globOptData.liveLossyInt32Syms);
@@ -6776,8 +6777,6 @@ GlobOpt::CanProveConditionalBranch(IR::Instr *instr, Value *src1Val, Value *src2
67766777
}
67776778
case Js::OpCode::BrFalse_I4:
67786779
{
6779-
// this path would probably work outside of asm.js, but we should verify that if we ever hit this scenario
6780-
Assert(GetIsAsmJSFunc());
67816780
constVal = 0;
67826781
if (!src1Val->GetValueInfo()->TryGetIntConstantValue(&constVal))
67836782
{

‎lib/Backend/GlobOpt.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -861,6 +861,7 @@ class GlobOpt
861861
void CaptureArguments(BasicBlock *block, BailOutInfo * bailOutInfo, JitArenaAllocator *allocator);
862862
void CaptureByteCodeSymUses(IR::Instr * instr);
863863
IR::ByteCodeUsesInstr * InsertByteCodeUses(IR::Instr * instr, bool includeDef = false);
864+
void ProcessInlineeEnd(IR::Instr * instr);
864865
void TrackCalls(IR::Instr * instr);
865866
void RecordInlineeFrameInfo(IR::Instr* instr);
866867
void EndTrackCall(IR::Instr * instr);

‎lib/Backend/GlobOptBailOut.cpp

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,19 @@ GlobOpt::CaptureByteCodeSymUses(IR::Instr * instr)
467467
"Instruction edited before capturing the byte code use");
468468
}
469469

470+
void
471+
GlobOpt::ProcessInlineeEnd(IR::Instr* instr)
472+
{
473+
if (instr->m_func->m_hasInlineArgsOpt)
474+
{
475+
RecordInlineeFrameInfo(instr);
476+
}
477+
EndTrackingOfArgObjSymsForInlinee();
478+
479+
Assert(this->currentBlock->globOptData.inlinedArgOutSize >= instr->GetArgOutSize(/*getInterpreterArgOutCount*/ false));
480+
this->currentBlock->globOptData.inlinedArgOutSize -= instr->GetArgOutSize(/*getInterpreterArgOutCount*/ false);
481+
}
482+
470483
void
471484
GlobOpt::TrackCalls(IR::Instr * instr)
472485
{
@@ -576,14 +589,7 @@ GlobOpt::TrackCalls(IR::Instr * instr)
576589
break;
577590

578591
case Js::OpCode::InlineeEnd:
579-
if (instr->m_func->m_hasInlineArgsOpt)
580-
{
581-
RecordInlineeFrameInfo(instr);
582-
}
583-
EndTrackingOfArgObjSymsForInlinee();
584-
585-
Assert(this->currentBlock->globOptData.inlinedArgOutSize >= instr->GetArgOutSize(/*getInterpreterArgOutCount*/ false));
586-
this->currentBlock->globOptData.inlinedArgOutSize -= instr->GetArgOutSize(/*getInterpreterArgOutCount*/ false);
592+
ProcessInlineeEnd(instr);
587593
break;
588594

589595
case Js::OpCode::InlineeMetaArg:

‎test/Optimizer/bugsimplepathbrfoldgetter.baseline

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
5
2+
5
3+
Done iterator1
4+
5+
5
6+
getter
7+
8+
getter
9+
10+
Done iterator2
11+
12+
5
13+
getter
14+
15+
getter
16+
17+
Done iterator3
18+
19+
obj1.prop1 getter
20+
obj1.prop1 getter
21+
obj1.prop1 getter

‎test/Optimizer/bugsimplepathbrfoldgetter.js

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
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 next1(arr, len) {
7+
var obj = {done:true, value:undefined};
8+
if (len > 5) {
9+
// cannot fold, because obj not defined in localValueTable
10+
return obj;
11+
}
12+
return {done:false, value:arr[len]};
13+
}
14+
15+
function iterator1(x) {
16+
var arr = [0,1,2,3,4,5,6,7,8,9,10];
17+
var res = next1(arr, x);
18+
if (!res.done) {
19+
print(res.value);
20+
}
21+
}
22+
23+
iterator1(5);
24+
iterator1(100);
25+
iterator1(5);
26+
print("Done iterator1\n");
27+
28+
function next2(arr, len) {
29+
if (len > 5) {
30+
var obj = {done:true, value:undefined};
31+
Object.defineProperty(obj, "done", {get : function() {print("getter\n"); return true;}});
32+
return obj;
33+
}
34+
return {done:false, value:arr[len]};
35+
}
36+
37+
function iterator2(x) {
38+
var arr = [0,1,2,3,4,5,6,7,8,9,10];
39+
var res = next2(arr, x);
40+
if (!res.done) {
41+
print(res.value);
42+
}
43+
}
44+
45+
iterator2(5);
46+
iterator2(100);
47+
iterator2(200);
48+
print("Done iterator2\n");
49+
50+
function next3(arr, len) {
51+
if (len > 5) {
52+
return {get done() { print("getter\n"); return true;}, value:undefined};
53+
}
54+
return {done:false, value:arr[len]};
55+
}
56+
57+
function iterator3(x) {
58+
var arr = [0,1,2,3,4,5,6,7,8,9,10];
59+
var res = next3(arr, x);
60+
if (!res.done) {
61+
print(res.value);
62+
}
63+
}
64+
65+
iterator3(5);
66+
iterator3(100);
67+
iterator3(200);
68+
print("Done iterator3\n");
69+
70+
function test0() {
71+
var obj1 = {};
72+
var arrObj0 = {};
73+
var func0 = function (x) {
74+
with (arrObj0) {
75+
if (x > 100) {
76+
obj1.prop1 = (Object.defineProperty(obj1, 'prop1', { get: function () { WScript.Echo('obj1.prop1 getter'); return 3; }, configurable: true }));
77+
}
78+
else {
79+
obj1.prop1 = 3;
80+
}
81+
true ? obj1.prop1 : obj1.prop1;
82+
}
83+
};
84+
func0(200);
85+
func0(200);
86+
func0(100);
87+
}
88+
test0();

‎test/Optimizer/rlexe.xml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1532,4 +1532,11 @@
15321532
<compile-flags> -loopinterpretcount:1 -bgjit- -maxsimplejitruncount:1 -maxinterpretcount:1 -oopjit-</compile-flags>
15331533
</default>
15341534
</test>
1535+
<test>
1536+
<default>
1537+
<files>bugsimplepathbrfoldgetter.js</files>
1538+
<baseline>bugsimplepathbrfoldgetter.baseline</baseline>
1539+
<tags>exclude_dynapogo,exclude_nonative</tags>
1540+
</default>
1541+
</test>
15351542
</regress-exe>

‎test/Optimizer/testsimplepathbrfold.baseline

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
100
22
TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 1 from Block 2 to Block 4 in func foo
3-
TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 2 from Block 4 to Block 3 in func foo
43
100
54
Done foo
65

@@ -10,6 +9,8 @@ TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 2 from B
109
Done bar
1110

1211
100
12+
TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 1 from Block 3 to Block 4 in func baz
13+
TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 2 from Block 3 to Block 5 in func baz
1314
100
1415
Done baz
1516

@@ -62,7 +63,6 @@ Done iterator
6263

6364
5
6465
TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 1 from Block 3 to Block 5 in func iterator2
65-
TRACE PathDependentBranchFolding: Can prove retarget of branch in Block 3 from Block 5 to Block 4 in func iterator2
6666
5
6767
Done iterator2
6868

0 commit comments

Comments
 (0)