[hir] Update mutable range of operands during LeaveSSA
It's not enough to only update the mutable range of the canonical id created instead of the phi but we need to update the mutable range of each of the operands of the phi as well to account for the fact that the phi could've been mutated later. The operands are updated only if the phi is mutated later. Otherwise these operands can be cached in their blocks. Fixes https://github.com/facebook/react-forget/issues/978
Sathya Gunasekaran committed
Jan 11, 2023 at 15:17 UTC
f9ecc96bf3a16d3a50d4a39b16af5174026ced5b
6 files changed
+183
-27
compiler/forget/src/SSA/LeaveSSA.ts
+32
-9
@@ -7,6 +7,8 @@
7
8
import invariant from "invariant";
9
import {
10
+ BasicBlock,
11
+ BlockId,
12
Effect,
13
GeneratedSource,
14
HIRFunction,
@@ -96,6 +98,16 @@ export function leaveSSA(fn: HIRFunction) {
98
// phi id or, more typically, the operand that was defined prior to the phi.
99
const rewrites: Map<Identifier, Identifier> = new Map();
100
101
+ type PhiState = {
102
+ phi: Phi;
103
+ block: BasicBlock;
104
+ };
105
+ function pushPhis(arr: Array<PhiState>, block: BasicBlock) {
106
+ for (const phi of block.phis) {
107
+ arr.push({ phi, block });
108
+ }
109
+ }
110
+
111
for (const [, block] of fn.body.blocks) {
112
invariant(
113
block.phis.size === 0,
@@ -105,8 +117,8 @@ export function leaveSSA(fn: HIRFunction) {
117
// Find any phi nodes which need a variable declaration in the current block
118
// This includes phis in fallthrough nodes, or blocks that form part of control flow
119
// such as for or while (and later if/switch).
108
- const reassignmentPhis: Array<Phi> = [];
109
- const rewritePhis: Array<Phi> = [];
120
+ const reassignmentPhis: Array<PhiState> = [];
121
+ const rewritePhis: Array<PhiState> = [];
122
const terminal = block.terminal;
123
if (
124
(terminal.kind === "if" ||
@@ -116,29 +128,29 @@ export function leaveSSA(fn: HIRFunction) {
128
terminal.fallthrough !== null
129
) {
130
const fallthrough = fn.body.blocks.get(terminal.fallthrough)!;
119
- reassignmentPhis.push(...fallthrough.phis);
131
+ pushPhis(reassignmentPhis, fallthrough);
132
fallthrough.phis.clear();
133
}
134
if (terminal.kind === "while" || terminal.kind === "for") {
135
const test = fn.body.blocks.get(terminal.test)!;
124
- rewritePhis.push(...test.phis);
136
+ pushPhis(rewritePhis, test);
137
test.phis.clear();
138
139
const loop = fn.body.blocks.get(terminal.loop)!;
128
- rewritePhis.push(...loop.phis);
140
+ pushPhis(rewritePhis, loop);
141
loop.phis.clear();
142
}
143
if (terminal.kind === "for") {
144
const init = fn.body.blocks.get(terminal.init)!;
133
- rewritePhis.push(...init.phis);
145
+ pushPhis(rewritePhis, init);
146
init.phis.clear();
147
148
const update = fn.body.blocks.get(terminal.update)!;
137
- rewritePhis.push(...update.phis);
149
+ pushPhis(rewritePhis, update);
150
update.phis.clear();
151
}
152
141
- for (const phi of reassignmentPhis) {
153
+ for (const { phi, block: phiBlock } of reassignmentPhis) {
154
// In some cases one of the phi operands can be defined *before* the let binding
155
// we will generate. For example, a variable that is only rebound in one branch of
156
// an if but not another. In this case we populate the let binding with this initial
@@ -178,6 +190,14 @@ export function leaveSSA(fn: HIRFunction) {
190
canonicalId.mutableRange.start = makeInstructionId(start);
191
canonicalId.mutableRange.end = makeInstructionId(end);
192
193
+ // If there are no instructions in the block then there's just a terminal
194
+ // node, which has no mutation, so that should be false.
195
+ //
196
+ // TODO(joe): This above statement is true, right? Could there be a value
197
+ // block with instructions in terminals?
198
+ const isPhiMutatedAfterCreation: boolean =
199
+ end > (phiBlock.instructions.at(0)?.id ?? end);
200
+
201
// If this phi id is the canonical id we need to generate a let binding for it
202
// (otherwise, it means this phi merges into some other phi which already generated
203
// a binding
@@ -220,6 +240,9 @@ export function leaveSSA(fn: HIRFunction) {
240
if (operand === initOperand) {
241
continue;
242
}
243
+ if (isPhiMutatedAfterCreation) {
244
+ operand.mutableRange.end = canonicalId.mutableRange.end;
245
+ }
246
const predecessor = fn.body.blocks.get(predecessorId)!;
247
const instr: Instruction = {
248
id: predecessor.terminal.id,
@@ -247,7 +270,7 @@ export function leaveSSA(fn: HIRFunction) {
270
// Similar logic for rewrite phis that occur in loops, except that instead of a new let binding
271
// we pick one of the operands as the canonical id, and rewrite all references to the other
272
// operands and the phi to reference this canonical id.
250
- for (const phi of rewritePhis) {
273
+ for (const { phi } of rewritePhis) {
274
let canonicalId = rewrites.get(phi.id);
275
if (canonicalId === undefined) {
276
canonicalId = phi.id;
compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.expect.md
new
+73
@@ -0,0 +1,73 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function foo(a, b, c, d) {
6
+ let x = {};
7
+ if (someVal) {
8
+ x = { b };
9
+ } else {
10
+ x = { c };
11
+ }
12
+
13
+ return x;
14
+}
15
+
16
+```
17
+
18
+## Code
19
+
20
+```javascript
21
+function foo(a, b, c, d) {
22
+ const $ = React.useMemoCache();
23
+ const x = {};
24
+ const c_0 = $[0] !== b;
25
+ const c_1 = $[1] !== c;
26
+ let x$0;
27
+ if (c_0 || c_1) {
28
+ x$0 = undefined;
29
+
30
+ if (someVal) {
31
+ const c_3 = $[3] !== b;
32
+ let x$1;
33
+
34
+ if (c_3) {
35
+ x$1 = {
36
+ b: b,
37
+ };
38
+ $[3] = b;
39
+ $[4] = x$1;
40
+ } else {
41
+ x$1 = $[4];
42
+ }
43
+
44
+ x$0 = x$1;
45
+ } else {
46
+ const c_5 = $[5] !== c;
47
+ let x$2;
48
+
49
+ if (c_5) {
50
+ x$2 = {
51
+ c: c,
52
+ };
53
+ $[5] = c;
54
+ $[6] = x$2;
55
+ } else {
56
+ x$2 = $[6];
57
+ }
58
+
59
+ x$0 = x$2;
60
+ }
61
+
62
+ $[0] = b;
63
+ $[1] = c;
64
+ $[2] = x$0;
65
+ } else {
66
+ x$0 = $[2];
67
+ }
68
+
69
+ return x$0;
70
+}
71
+
72
+```
73
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.js
new
+10
@@ -0,0 +1,10 @@
1
+function foo(a, b, c, d) {
2
+ let x = {};
3
+ if (someVal) {
4
+ x = { b };
5
+ } else {
6
+ x = { c };
7
+ }
8
+
9
+ return x;
10
+}
compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.expect.md
new
+55
@@ -0,0 +1,55 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function foo(a, b, c, d) {
6
+ let x = {};
7
+ if (someVal) {
8
+ x = { b };
9
+ } else {
10
+ x = { c };
11
+ }
12
+
13
+ x.f = 1;
14
+ return x;
15
+}
16
+
17
+```
18
+
19
+## Code
20
+
21
+```javascript
22
+function foo(a, b, c, d) {
23
+ const $ = React.useMemoCache();
24
+ const x = {};
25
+ const c_0 = $[0] !== b;
26
+ const c_1 = $[1] !== c;
27
+ let x$0;
28
+ if (c_0 || c_1) {
29
+ x$0 = undefined;
30
+
31
+ if (someVal) {
32
+ const x$1 = {
33
+ b: b,
34
+ };
35
+ x$0 = x$1;
36
+ } else {
37
+ const x$2 = {
38
+ c: c,
39
+ };
40
+ x$0 = x$2;
41
+ }
42
+
43
+ x$0.f = 1;
44
+ $[0] = b;
45
+ $[1] = c;
46
+ $[2] = x$0;
47
+ } else {
48
+ x$0 = $[2];
49
+ }
50
+
51
+ return x$0;
52
+}
53
+
54
+```
55
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.js
new
+11
@@ -0,0 +1,11 @@
1
+function foo(a, b, c, d) {
2
+ let x = {};
3
+ if (someVal) {
4
+ x = { b };
5
+ } else {
6
+ x = { c };
7
+ }
8
+
9
+ x.f = 1;
10
+ return x;
11
+}
compiler/forget/src/__tests__/fixtures/hir/obj-mutated-after-if-else.expect.md
+2
-18
@@ -28,26 +28,10 @@ function foo(a, b, c, d) {
28
x$0 = undefined;
29
30
if (a) {
31
- let x$1;
32
-
33
- if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
34
- x$1 = someObj();
35
- $[2] = x$1;
36
- } else {
37
- x$1 = $[2];
38
- }
39
-
31
+ const x$1 = someObj();
32
x$0 = x$1;
33
} else {
42
- let x$2;
43
-
44
- if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
45
- x$2 = someObj();
46
- $[3] = x$2;
47
- } else {
48
- x$2 = $[3];
49
- }
50
-
34
+ const x$2 = someObj();
35
x$0 = x$2;
36
}
37