Skip to content

Commit 934b1e2

Browse files
kmiller68kmiller68
committed
Conditional writes are incorrectly handled in Array allocation sinking
https://bugs.webkit.org/show_bug.cgi?id=299956 rdar://161681941 Reviewed by Yusuke Suzuki and Yijia Huang. The current bottom value in ObjectAllocationSinking is incorrect for arrays. Unlike with objects, which track conditional stores by passing the active structure through SSA, arrays can't do this. Instead we should set default value to the appropriate hole value for the given IndexingShape. To make this work I had to fix some Phi/Upsilon ResultFormat bugs since they previously assumed everything would be a JSValue. Also, add ASSERT to FTL lowering that the Phi/Upsilon formats match. I spent 1/2 a day trying to understand why I was getting zero, when the issue was those values disagreed and I was getting the default zero value. Tests: JSTests/stress/array-allocation-sink-conditional-write-osr.js JSTests/stress/array-sink-materialize-conditional-write-argument-value.js JSTests/stress/array-sink-materialize-conditional-write.js Canonical link: https://commits.webkit.org/300888@main
1 parent 6e72c32 commit 934b1e2

File tree

6 files changed

+344
-15
lines changed

6 files changed

+344
-15
lines changed

‎JSTests/stress/array-allocation-sink-conditional-write-osr.js‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
function test(write, escape) {
2+
let arr = new Array(4);
3+
4+
arr[1] = 1.1;
5+
6+
if (escape) {
7+
return arr;
8+
}
9+
10+
if (write) {
11+
arr[0] = 2;
12+
}
13+
14+
}
15+
noInline(test);
16+
17+
function test2(write, escape) {
18+
let arr = new Array(4);
19+
20+
arr[1] = 1;
21+
22+
if (escape) {
23+
return arr;
24+
}
25+
26+
if (write) {
27+
arr[0] = 2;
28+
}
29+
}
30+
noInline(test2);
31+
32+
function test3(write, escape) {
33+
let arr = new Array(4);
34+
35+
arr[1] = {};
36+
37+
if (escape) {
38+
return arr;
39+
}
40+
41+
if (write) {
42+
arr[0] = 2;
43+
}
44+
}
45+
noInline(test3);
46+
47+
function test4(overwrite, escape) {
48+
let arr = new Array(4);
49+
if (escape) {
50+
return arr;
51+
}
52+
arr[0] = 1.1;
53+
if (overwrite) {
54+
arr[0] = 3.14;
55+
}
56+
}
57+
noInline(test4);
58+
59+
// JIT warmup loop
60+
for(let i = 0; i < testLoopCount; i++) {
61+
test(true, false);
62+
test(false, false);
63+
test2(true, false);
64+
test2(false, false);
65+
test3(true, false);
66+
test3(false, false);
67+
test4(true, false);
68+
test4(false, false);
69+
}
70+
71+
function check(functor) {
72+
let noWrite = functor(true, true);
73+
if (0 in noWrite)
74+
throw new Error(noWrite);
75+
}
76+
77+
check(test);
78+
check(test2);
79+
check(test3);
80+
81+
if (0 in test4(true, true))
82+
throw new Error();
83+
84+
if (0 in test4(false, true))
85+
throw new Error();
86+

‎JSTests/stress/array-sink-materialize-conditional-write-argument-value.js‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
function test1(write, escape, value) {
2+
let arr = new Array(4);
3+
4+
arr[1] = 1;
5+
6+
if (write) {
7+
arr[0] = value + 3;
8+
}
9+
10+
if (escape) {
11+
return arr;
12+
}
13+
}
14+
noInline(test1);
15+
16+
function test2(write, escape, value) {
17+
let arr = new Array(4);
18+
19+
arr[1] = 1.1;
20+
21+
if (write) {
22+
arr[0] = value + 3;
23+
}
24+
25+
if (escape) {
26+
return arr;
27+
}
28+
}
29+
noInline(test2);
30+
31+
function test3(write, escape, value) {
32+
let arr = new Array(4);
33+
34+
arr[1] = {};
35+
36+
if (write) {
37+
arr[0] = value + 3;
38+
}
39+
40+
if (escape) {
41+
return arr;
42+
}
43+
}
44+
noInline(test3);
45+
46+
function test4(write, escape, value) {
47+
let arr = new Array(4);
48+
49+
arr[1] = {};
50+
51+
if (write) {
52+
arr[0] = {
53+
value: value + 3,
54+
valueOf() { return this.value; },
55+
};
56+
}
57+
58+
if (escape) {
59+
return arr;
60+
}
61+
}
62+
noInline(test4);
63+
64+
function test5(write, escape, value) {
65+
let arr = new Array(4);
66+
67+
arr[1] = {};
68+
69+
if (write) {
70+
arr[0] = {
71+
value: value + 3,
72+
valueOf() { return this.value; },
73+
// Add a reference cycle.
74+
arr,
75+
};
76+
}
77+
78+
if (escape) {
79+
return arr;
80+
}
81+
}
82+
noInline(test5);
83+
84+
// JIT warmup loop
85+
for(let i = 0; i < testLoopCount; i++) {
86+
test1(true, false, 3);
87+
test1(false, true, 3);
88+
test2(true, false, 3);
89+
test2(false, true, 3);
90+
test3(true, false, 3);
91+
test3(false, true, 3);
92+
test4(true, false, 3);
93+
test4(false, true, 3);
94+
test5(true, false, 3);
95+
test5(false, true, 3);
96+
}
97+
98+
function check(functor, value) {
99+
let write = functor(true, true, value);
100+
if (write[0] != value + 3)
101+
throw new Error(write);
102+
103+
let noWrite = functor(false, true, value);
104+
if (0 in noWrite)
105+
throw new Error(write);
106+
}
107+
108+
check(test1, 3);
109+
check(test2, 3);
110+
check(test3, 3);
111+
check(test4, 3);
112+
check(test5, 3);

‎JSTests/stress/array-sink-materialize-conditional-write.js‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
function test(write, escape) {
2+
let arr = new Array(4);
3+
4+
arr[1] = 1.1;
5+
6+
if (write) {
7+
arr[0] = 2;
8+
}
9+
10+
if (escape) {
11+
return arr;
12+
}
13+
}
14+
noInline(test);
15+
16+
function test2(write, escape) {
17+
let arr = new Array(4);
18+
19+
arr[1] = 1;
20+
21+
if (write) {
22+
arr[0] = 2;
23+
}
24+
25+
if (escape) {
26+
return arr;
27+
}
28+
}
29+
noInline(test2);
30+
31+
function test3(write, escape) {
32+
let arr = new Array(4);
33+
34+
arr[1] = {};
35+
36+
if (write) {
37+
arr[0] = 2;
38+
}
39+
40+
if (escape) {
41+
return arr;
42+
}
43+
}
44+
noInline(test3);
45+
46+
function test4(overwrite, escape) {
47+
let arr = new Array(4);
48+
arr[0] = 1.1;
49+
if (overwrite) {
50+
arr[0] = 3.14;
51+
}
52+
if (escape) {
53+
return arr;
54+
}
55+
}
56+
noInline(test4);
57+
58+
// JIT warmup loop
59+
for(let i = 0; i < testLoopCount; i++) {
60+
test(true, false);
61+
test(false, true);
62+
test2(true, false);
63+
test2(false, true);
64+
test3(true, false);
65+
test3(false, true);
66+
test4(true, false);
67+
test4(false, true);
68+
}
69+
70+
function check(functor) {
71+
let write = functor(true, true);
72+
if (write[0] !== 2)
73+
throw new Error(write);
74+
75+
let noWrite = functor(false, true);
76+
if (0 in noWrite)
77+
throw new Error(noWrite);
78+
}
79+
80+
check(test);
81+
check(test2);
82+
check(test3);
83+
84+
if (test4(true, true)[0] !== 3.14)
85+
throw new Error();
86+
87+
if (test4(false, true)[0] !== 1.1)
88+
throw new Error();
89+

‎Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3723,15 +3723,16 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
37233723
case MaterializeNewArrayWithButterfly: {
37243724
SpeculatedType validTypes = [&]() {
37253725
switch (node->indexingType()) {
3726-
case ALL_INT32_INDEXING_TYPES: return SpecInt32Only;
3726+
// We can get JSValue() (aka SpecEmpty) when the property hasn't been initialized yet and is still a hole.
3727+
case ALL_INT32_INDEXING_TYPES: return SpecInt32Only | SpecEmpty;
37273728
case ALL_DOUBLE_INDEXING_TYPES: return SpecBytecodeNumber;
37283729
case ALL_CONTIGUOUS_INDEXING_TYPES: return SpecBytecodeTop;
37293730
default: break;
37303731
}
37313732
RELEASE_ASSERT_NOT_REACHED();
37323733
}();
37333734
for (unsigned i = 2; i < node->numChildren(); ++i)
3734-
RELEASE_ASSERT(isSubtypeSpeculation(forNode(m_graph.varArgChild(node, i)).m_type, validTypes));
3735+
DFG_ASSERT(m_graph, node, isSubtypeSpeculation(forNode(m_graph.varArgChild(node, i)).m_type, validTypes));
37353736

37363737
[[fallthrough]];
37373738
}

0 commit comments

Comments
 (0)