LeaveSSA: consistently rename identifiers even for value block phis
This PR clarifies the logic for adjust mutable ranges of phis and their operands during LeaveSSA. Previously we had logic in several places to determine whether/how to extend the ranges of each phi and its operands: this occurred while traversing reassignmentPhis (in 2+ places) and rewritePhis, as well as in rewritePlace(). This was kind of a band-aid to make things work, but the logic was imprecise. The actual rules are as follows: If there is a back-edge, or the phi id is unnamed, then were extend the ranges of the phi and its operands to min(starts) and max(ends). This ensures that the operands are computed as one unit, ie put into a single reactive scope. For loops this is necessary because...looping! For unnamed values this is necessary because of the way we collapse logical and ternary expressions back to a hierarchical ReactiveFunction — we need to make sure the final mutable range extends from the start of the final instruction up to the end of the logical/ternaries value blocks. Otherwise this is a phi where operands come from predecessors and are named. If the phi is mutated later, then we have to extend the end of each operand's range to account for the fact that they can be mutated later. Else, we leave the operands alone. Behavior doesn't change, but we consolidate all of the mutable range logic in one place.
Joe Savona committed
Mar 14, 2023 at 15:24 UTC
59cd1ca569c25d35c0aae013936c9941c81a251e
2 files changed
+97
-59
compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts
+1
-1
@@ -97,8 +97,8 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
97
scopeIdentifiers.union([phi.id, phiId]);
98
}
99
}
100
- block.phis.clear();
100
}
101
+ block.phis.clear();
102
103
for (const instr of block.instructions) {
104
const operands: Array<Identifier> = [];
compiler/forget/src/SSA/LeaveSSA.ts
+96
-58
@@ -9,12 +9,12 @@ import invariant from "invariant";
9
import { CompilerError } from "../CompilerError";
10
import {
11
BasicBlock,
12
+ BlockId,
13
Effect,
14
GeneratedSource,
15
HIRFunction,
16
Identifier,
17
Instruction,
17
- InstructionId,
18
InstructionKind,
19
LValue,
20
LValuePattern,
@@ -108,10 +108,19 @@ export function leaveSSA(fn: HIRFunction): void {
108
phi: Phi;
109
block: BasicBlock;
110
};
111
- function pushPhis(arr: Array<PhiState>, block: BasicBlock): void {
111
+
112
+ const seen = new Set<BlockId>();
113
+ const backEdgePhis = new Set<Phi>();
114
+ for (const [, block] of fn.body.blocks) {
115
for (const phi of block.phis) {
113
- arr.push({ phi, block });
116
+ for (const [pred] of phi.operands) {
117
+ if (!seen.has(pred)) {
118
+ backEdgePhis.add(phi);
119
+ break;
120
+ }
121
+ }
122
}
123
+ seen.add(block.id);
124
}
125
126
for (const [, block] of fn.body.blocks) {
@@ -124,7 +133,10 @@ export function leaveSSA(fn: HIRFunction): void {
133
const originalLVal = declarations.get(
134
value.lvalue.place.identifier.name
135
);
127
- if (originalLVal === undefined) {
136
+ if (
137
+ originalLVal === undefined ||
138
+ originalLVal.lvalue === value.lvalue // in case this was pre-declared for the `for` initializer
139
+ ) {
140
declarations.set(value.lvalue.place.identifier.name, {
141
lvalue: value.lvalue,
142
place: value.lvalue.place,
@@ -134,13 +146,10 @@ export function leaveSSA(fn: HIRFunction): void {
146
// This is an instance of the original id, so we need to promote the original declaration
147
// to a `let` and the current lval to a `reassign`
148
originalLVal.lvalue.kind = InstructionKind.Let;
149
+ value.lvalue.kind = InstructionKind.Reassign;
150
}
151
} else if (rewrites.has(value.lvalue.place.identifier)) {
139
- value.lvalue.kind =
140
- rewrites.get(value.lvalue.place.identifier) ===
141
- value.lvalue.place.identifier
142
- ? InstructionKind.Let
143
- : InstructionKind.Reassign;
152
+ value.lvalue.kind = InstructionKind.Const;
153
}
154
} else if (value.kind === "Destructure") {
155
let kind: InstructionKind | null = null;
@@ -157,7 +166,10 @@ export function leaveSSA(fn: HIRFunction): void {
166
kind = InstructionKind.Const;
167
} else {
168
const originalLVal = declarations.get(place.identifier.name);
160
- if (originalLVal === undefined) {
169
+ if (
170
+ originalLVal === undefined ||
171
+ originalLVal.lvalue === value.lvalue
172
+ ) {
173
declarations.set(place.identifier.name, {
174
lvalue: value.lvalue,
175
place,
@@ -207,6 +219,51 @@ export function leaveSSA(fn: HIRFunction): void {
219
// such as for or while (and later if/switch).
220
const reassignmentPhis: Array<PhiState> = [];
221
const rewritePhis: Array<PhiState> = [];
222
+ function pushPhis(phiBlock: BasicBlock): void {
223
+ for (const phi of phiBlock.phis) {
224
+ if (phi.id.name === null) {
225
+ rewritePhis.push({ phi, block: phiBlock });
226
+ } else {
227
+ reassignmentPhis.push({ phi, block: phiBlock });
228
+ }
229
+ const hasBackEdge = backEdgePhis.has(phi);
230
+ const isPhiMutatedAfterCreation: boolean =
231
+ phi.id.mutableRange.end >
232
+ (phiBlock.instructions.at(0)?.id ?? phiBlock.terminal.id);
233
+
234
+ // Named variables whose phi doesn't have a back-edge can potentially be independenly
235
+ // memoized, depending on whether the phi is after its creation.
236
+ if (phi.id.name !== null && !hasBackEdge) {
237
+ if (!isPhiMutatedAfterCreation) {
238
+ // Simple case: predecesor-only values flowing into a phi, which is never modified:
239
+ // adjust the phi's range to clarify that the identifier does not mutate
240
+ phi.id.mutableRange.start = terminal.id;
241
+ phi.id.mutableRange.end = makeInstructionId(terminal.id + 1);
242
+ } else {
243
+ // Predecessor only values flow into a phi, which is modified later:
244
+ // all operands flow into the phi and can be modified, must extend their ranges
245
+ for (const [, operand] of phi.operands) {
246
+ operand.mutableRange.end = phi.id.mutableRange.end;
247
+ }
248
+ }
249
+ return;
250
+ }
251
+ // Otherwise this is a temporary phi (logical or ternary) or occurs in a loop. In either
252
+ // case we can't independently memoize any of the values: unify their ranges to span the
253
+ // min(start) to max(end) so that we create a single scope for all the computation.
254
+ let start = block.terminal.id as number;
255
+ let end = Number.MIN_SAFE_INTEGER;
256
+ const operands = [phi.id, ...phi.operands.values()];
257
+ for (const operand of operands) {
258
+ start = Math.min(start, operand.mutableRange.start);
259
+ end = Math.max(end, operand.mutableRange.end);
260
+ }
261
+ for (const operand of operands) {
262
+ operand.mutableRange.start = makeInstructionId(start);
263
+ operand.mutableRange.end = makeInstructionId(end);
264
+ }
265
+ }
266
+ }
267
if (
268
(terminal.kind === "if" ||
269
terminal.kind === "switch" ||
@@ -215,36 +272,47 @@ export function leaveSSA(fn: HIRFunction): void {
272
terminal.fallthrough !== null
273
) {
274
const fallthrough = fn.body.blocks.get(terminal.fallthrough)!;
218
- for (const phi of fallthrough.phis) {
219
- if (phi.id.name == null) {
220
- rewritePhis.push({ phi, block: fallthrough });
221
- } else {
222
- reassignmentPhis.push({ phi, block: fallthrough });
223
- }
224
- }
275
+ pushPhis(fallthrough);
276
}
277
if (terminal.kind === "while" || terminal.kind === "for") {
278
const test = fn.body.blocks.get(terminal.test)!;
228
- pushPhis(rewritePhis, test);
229
- test.phis.clear();
279
+ pushPhis(test);
280
281
const loop = fn.body.blocks.get(terminal.loop)!;
232
- pushPhis(rewritePhis, loop);
233
- loop.phis.clear();
282
+ pushPhis(loop);
283
}
284
if (terminal.kind === "for") {
285
const init = fn.body.blocks.get(terminal.init)!;
237
- pushPhis(rewritePhis, init);
238
- init.phis.clear();
286
+ pushPhis(init);
287
+
288
+ // To avoid generating a let binding for the initializer prior to the loop,
289
+ // check to see if the for declares an iterator variable
290
+ const initIdentifier = init.instructions.at(-1);
291
+ if (
292
+ initIdentifier !== undefined &&
293
+ initIdentifier.value.kind === "StoreLocal"
294
+ ) {
295
+ const value = initIdentifier.value;
296
+ if (value.lvalue.place.identifier.name !== null) {
297
+ const originalLVal = declarations.get(
298
+ value.lvalue.place.identifier.name
299
+ );
300
+ if (originalLVal === undefined) {
301
+ declarations.set(value.lvalue.place.identifier.name, {
302
+ lvalue: value.lvalue,
303
+ place: value.lvalue.place,
304
+ });
305
+ value.lvalue.kind = InstructionKind.Const;
306
+ }
307
+ }
308
+ }
309
310
const update = fn.body.blocks.get(terminal.update)!;
241
- pushPhis(rewritePhis, update);
242
- update.phis.clear();
311
+ pushPhis(update);
312
}
313
if (terminal.kind === "logical" || terminal.kind === "ternary") {
314
const fallthrough = fn.body.blocks.get(terminal.fallthrough)!;
246
- pushPhis(rewritePhis, fallthrough);
247
- fallthrough.phis.clear();
315
+ pushPhis(fallthrough);
316
}
317
318
for (const { phi, block: phiBlock } of reassignmentPhis) {
@@ -267,12 +335,6 @@ export function leaveSSA(fn: HIRFunction): void {
335
phi.id.mutableRange.end >
336
(phiBlock.instructions.at(0)?.id ?? phiBlock.terminal.id);
337
270
- // If a phi is never mutated after creation, reset its mutable range to be itself
271
- if (!isPhiMutatedAfterCreation) {
272
- phi.id.mutableRange.start = terminal.id;
273
- phi.id.mutableRange.end = makeInstructionId(terminal.id + 1);
274
- }
275
-
338
// If we never saw a declaration for this phi, it may have been pruned by DCE, so synthesize
339
// a new Let binding
340
invariant(
@@ -361,9 +423,6 @@ export function leaveSSA(fn: HIRFunction): void {
423
block.instructions.push(instr);
424
declarations.set(phi.id.name, { lvalue, place: lvalue.place });
425
phi.id.mutableRange.start = terminal.id;
364
- if (!isPhiMutatedAfterCreation) {
365
- phi.id.mutableRange.end = makeInstructionId(terminal.id + 1);
366
- }
426
} else if (isPhiMutatedAfterCreation) {
427
// The declaration is not guaranteed to flow into the phi, for example in the case of a variable
428
// that is reassigned in all control flow paths to a given phi. The original declaration's range
@@ -389,10 +448,6 @@ export function leaveSSA(fn: HIRFunction): void {
448
canonicalId = canonicalOperand;
449
}
450
}
392
- canonicalId.mutableRange.start = Math.min(
393
- canonicalId.mutableRange.start,
394
- terminal.id
395
- ) as InstructionId;
451
rewrites.set(phi.id, canonicalId);
452
453
if (canonicalId.name !== null) {
@@ -404,17 +459,9 @@ export function leaveSSA(fn: HIRFunction): void {
459
}
460
461
// all versions of the variable need to be remapped to the canonical id
407
- // also extend the mutable range of the canonical id based on the min/max
408
- // of the ranges of its operands
409
- let start = canonicalId.mutableRange.start as number;
410
- let end = canonicalId.mutableRange.end as number;
462
for (const [, operand] of phi.operands) {
412
- start = Math.min(start, operand.mutableRange.start);
413
- end = Math.max(end, operand.mutableRange.end);
463
rewrites.set(operand, canonicalId);
464
}
416
- canonicalId.mutableRange.start = makeInstructionId(start);
417
- canonicalId.mutableRange.end = makeInstructionId(end);
465
}
466
}
467
}
@@ -432,20 +479,11 @@ function rewritePlace(
479
480
if (nextIdentifier !== undefined) {
481
if (nextIdentifier === prevIdentifier) return;
435
- nextIdentifier.mutableRange.start = makeInstructionId(
436
- Math.min(
437
- nextIdentifier.mutableRange.start,
438
- prevIdentifier.mutableRange.start
439
- )
440
- );
441
- nextIdentifier.mutableRange.end = makeInstructionId(
442
- Math.max(nextIdentifier.mutableRange.end, prevIdentifier.mutableRange.end)
443
- );
482
place.identifier = nextIdentifier;
483
} else if (prevIdentifier.name != null) {
484
const declaration = declarations.get(prevIdentifier.name);
447
- if (declaration === undefined) return;
485
// Only rewrite identifiers that were declared within the function
486
+ if (declaration === undefined) return;
487
const originalIdentifier = declaration.place.identifier;
488
prevIdentifier.id = originalIdentifier.id;
489
}