Skip to content

Commit 7ff08c9

Browse files
committed
[MERGE #5922 @wyrichte] Mult optimization in MemOp bug fix
Merge pull request #5922 from wyrichte:build/wyrichte/49152 ``` function f(a, s, e) { for (var i = s, j = s; i < e; i++, j++) { j = j + 1 // chakra notices that we can still do the MemOp optimization, it does so by... a[i] = 1 } // multiplying "j" by 2 here. print(j) } ``` The issue is that this "mult" instruction (multiply "j" by 2 after the loop in the example above) was inserted incorrectly into the IR: pseudo IR code: 1| j = j + range 2| MemOp 3| range = ((end - start) * 2) This optimization (accounting for situations like j = j + 1 from above) leads to bugs that incorrectly restore j's value (ex: j = j + 2 led to range == 0 when it came time to restore j's value, leading to "j" not being restored at all)
2 parents d77ff3e + f0da7b6 commit 7ff08c9

File tree

5 files changed

+135
-9
lines changed

5 files changed

+135
-9
lines changed

lib/Backend/BackwardPass.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8849,19 +8849,22 @@ BackwardPass::RestoreInductionVariableValuesAfterMemOp(Loop *loop)
88498849

88508850
IR::Opnd *inductionVariableOpnd = IR::RegOpnd::New(sym, IRType::TyInt32, localFunc);
88518851
IR::Opnd *tempInductionVariableOpnd = IR::RegOpnd::New(IRType::TyInt32, localFunc);
8852-
IR::Opnd *sizeOpnd = globOpt->GenerateInductionVariableChangeForMemOp(loop, inductionVariableChangeInfo.unroll);
88538852

88548853
// The induction variable is restored to a temp register before the MemOp occurs. Once the MemOp is
88558854
// complete, the induction variable's register is set to the value of the temp register. This is done
88568855
// in order to avoid overwriting the induction variable's value after a bailout on the MemOp.
8857-
IR::Instr* restoreInductionVarToTemp = IR::Instr::New(opCode, tempInductionVariableOpnd, inductionVariableOpnd, sizeOpnd, loop->GetFunc());
8858-
IR::Instr* restoreInductionVar = IR::Instr::New(Js::OpCode::Ld_A, inductionVariableOpnd, tempInductionVariableOpnd, loop->GetFunc());
8856+
IR::Instr* restoreInductionVarToTemp = IR::Instr::New(opCode, tempInductionVariableOpnd, inductionVariableOpnd, loop->GetFunc());
88598857

88608858
// The IR that restores the induction variable's value is placed before the MemOp. Since this IR can
88618859
// bailout to the loop's landing pad, placing this IR before the MemOp avoids performing the MemOp,
88628860
// bailing out because of this IR, and then performing the effects of the loop again.
88638861
loop->landingPad->InsertInstrBefore(restoreInductionVarToTemp, loop->memOpInfo->instr);
88648862

8863+
// The amount to be added or subtraced (depends on opCode) to the induction vairable after the MemOp.
8864+
IR::Opnd *sizeOpnd = globOpt->GenerateInductionVariableChangeForMemOp(loop, inductionVariableChangeInfo.unroll, restoreInductionVarToTemp);
8865+
restoreInductionVarToTemp->SetSrc2(sizeOpnd);
8866+
IR::Instr* restoreInductionVar = IR::Instr::New(Js::OpCode::Ld_A, inductionVariableOpnd, tempInductionVariableOpnd, loop->GetFunc());
8867+
88658868
// If restoring an induction variable results in an overflow, bailout to the loop's landing pad.
88668869
restoreInductionVarToTemp->ConvertToBailOutInstr(loop->bailOutInfo, IR::BailOutOnOverflow);
88678870

lib/Backend/GlobOpt.cpp

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2230,7 +2230,13 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
22302230
{
22312231
Loop::InductionVariableChangeInfo inductionVariableChangeInfo = { 0, 0 };
22322232
inductionVariableChangeInfo = loop->memOpInfo->inductionVariableChangeInfoMap->Lookup(inductionSymID, inductionVariableChangeInfo);
2233-
inductionVariableChangeInfo.unroll++;
2233+
2234+
// If inductionVariableChangeInfo.unroll has been invalidated, do
2235+
// not modify the Js::Constants::InvalidLoopUnrollFactor value
2236+
if (inductionVariableChangeInfo.unroll != Js::Constants::InvalidLoopUnrollFactor)
2237+
{
2238+
inductionVariableChangeInfo.unroll++;
2239+
}
22342240
inductionVariableChangeInfo.isIncremental = isIncr;
22352241
loop->memOpInfo->inductionVariableChangeInfoMap->Item(inductionSymID, inductionVariableChangeInfo);
22362242
}
@@ -16865,12 +16871,12 @@ GlobOpt::GenerateInductionVariableChangeForMemOp(Loop *loop, byte unroll, IR::In
1686516871

1686616872
IR::Opnd *unrollOpnd = IR::IntConstOpnd::New(unroll, type, localFunc);
1686716873

16868-
InsertInstr(IR::Instr::New(Js::OpCode::Mul_I4,
16869-
sizeOpnd,
16870-
loopCountOpnd,
16871-
unrollOpnd,
16872-
localFunc));
16874+
IR::Instr *inductionChangeMultiplier = IR::Instr::New(
16875+
Js::OpCode::Mul_I4, sizeOpnd, loopCountOpnd, unrollOpnd, localFunc);
16876+
16877+
InsertInstr(inductionChangeMultiplier);
1687316878

16879+
inductionChangeMultiplier->ConvertToBailOutInstr(loop->bailOutInfo, IR::BailOutOnOverflow);
1687416880
}
1687516881
}
1687616882
else

test/loop/MemOp.baseline

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
200
2+
2147483649
3+
30
4+
300
5+
2147483650
6+
40
7+
0
8+
2147483647
9+
10
10+
0
11+
2147483647
12+
10
13+
0
14+
2147483647
15+
10
16+
100
17+
2147483648
18+
20

test/loop/MemOp.js

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
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 f0(a, start, end) {
7+
for (var i = start, j = start; i < end; i++, j++) {
8+
j = j + 1
9+
a[i] = 1
10+
}
11+
a[j]
12+
print(j)
13+
14+
}
15+
16+
function f1(a, start, end) {
17+
for (var i = start, j = start; i < end; i++, j++) {
18+
j = j + 2
19+
a[i] = 1
20+
}
21+
a[j]
22+
print(j)
23+
24+
}
25+
26+
function f2(a, start, end) {
27+
for (var i = start, j = start; i >= end; i--, j--) {
28+
j = j - 1
29+
a[i] = 1
30+
}
31+
a[j]
32+
print(j)
33+
}
34+
35+
function f3(a, start, end) {
36+
for (var i = start, j = start; i >= end; i--, j--) {
37+
j = j - 2
38+
a[i] = 1
39+
}
40+
a[j]
41+
print(j)
42+
}
43+
44+
function f4(a, start, end) {
45+
for (var i = start, j = start; i < end; i++, j--) {
46+
j = j + 1
47+
a[i] = 1
48+
}
49+
a[j]
50+
print(j)
51+
}
52+
53+
function f5(a, start, end) {
54+
for (var i = start, j = start; i < end; i++, j--) {
55+
j = j + 2
56+
a[i] = 1
57+
}
58+
a[j]
59+
print(j)
60+
}
61+
62+
var a = new Float64Array(0xfff);
63+
f0(a, 0, 100)
64+
f0(a, 0x7fffffff, 0x80000000)
65+
f0(a, 10, 20)
66+
67+
f1(a, 0, 100)
68+
f1(a, 0x7fffffff, 0x80000000)
69+
f1(a, 10, 20)
70+
71+
f2(a, 0, 100)
72+
f2(a, 0x7fffffff, 0x80000000)
73+
f2(a, 10, 20)
74+
75+
f3(a, 0, 100)
76+
f3(a, 0x7fffffff, 0x80000000)
77+
f3(a, 10, 20)
78+
79+
f4(a, 0, 100)
80+
f4(a, 0x7fffffff, 0x80000000)
81+
f4(a, 10, 20)
82+
83+
f5(a, 0, 100)
84+
f5(a, 0x7fffffff, 0x80000000)
85+
f5(a, 10, 20)

test/loop/rlexe.xml

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,18 @@
4646
<files>infinite.js</files>
4747
</default>
4848
</test>
49+
<test>
50+
<default>
51+
<files>MemOp.js</files>
52+
<compile-flags>-off:simplejit -mic:1 -bgjit-</compile-flags>
53+
<baseline>MemOp.baseline</baseline>
54+
</default>
55+
</test>
56+
<test>
57+
<default>
58+
<files>MemOp.js</files>
59+
<compile-flags></compile-flags>
60+
<baseline>MemOp.baseline</baseline>
61+
</default>
62+
</test>
4963
</regress-exe>

0 commit comments

Comments
 (0)