From 59cd1ca569c25d35c0aae013936c9941c81a251e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 14 Mar 2023 15:24:38 -0700 Subject: [PATCH] LeaveSSA: consistently rename identifiers even for value block phis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../InferReactiveScopeVariables.ts | 2 +- compiler/forget/src/SSA/LeaveSSA.ts | 154 +++++++++++------- 2 files changed, 97 insertions(+), 59 deletions(-) diff --git a/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts index b00169e4c1..379e6032f6 100644 --- a/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -97,8 +97,8 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { scopeIdentifiers.union([phi.id, phiId]); } } - block.phis.clear(); } + block.phis.clear(); for (const instr of block.instructions) { const operands: Array = []; diff --git a/compiler/forget/src/SSA/LeaveSSA.ts b/compiler/forget/src/SSA/LeaveSSA.ts index 1acd79c4bf..350b0ad141 100644 --- a/compiler/forget/src/SSA/LeaveSSA.ts +++ b/compiler/forget/src/SSA/LeaveSSA.ts @@ -9,12 +9,12 @@ import invariant from "invariant"; import { CompilerError } from "../CompilerError"; import { BasicBlock, + BlockId, Effect, GeneratedSource, HIRFunction, Identifier, Instruction, - InstructionId, InstructionKind, LValue, LValuePattern, @@ -108,10 +108,19 @@ export function leaveSSA(fn: HIRFunction): void { phi: Phi; block: BasicBlock; }; - function pushPhis(arr: Array, block: BasicBlock): void { + + const seen = new Set(); + const backEdgePhis = new Set(); + for (const [, block] of fn.body.blocks) { for (const phi of block.phis) { - arr.push({ phi, block }); + for (const [pred] of phi.operands) { + if (!seen.has(pred)) { + backEdgePhis.add(phi); + break; + } + } } + seen.add(block.id); } for (const [, block] of fn.body.blocks) { @@ -124,7 +133,10 @@ export function leaveSSA(fn: HIRFunction): void { const originalLVal = declarations.get( value.lvalue.place.identifier.name ); - if (originalLVal === undefined) { + if ( + originalLVal === undefined || + originalLVal.lvalue === value.lvalue // in case this was pre-declared for the `for` initializer + ) { declarations.set(value.lvalue.place.identifier.name, { lvalue: value.lvalue, place: value.lvalue.place, @@ -134,13 +146,10 @@ export function leaveSSA(fn: HIRFunction): void { // This is an instance of the original id, so we need to promote the original declaration // to a `let` and the current lval to a `reassign` originalLVal.lvalue.kind = InstructionKind.Let; + value.lvalue.kind = InstructionKind.Reassign; } } else if (rewrites.has(value.lvalue.place.identifier)) { - value.lvalue.kind = - rewrites.get(value.lvalue.place.identifier) === - value.lvalue.place.identifier - ? InstructionKind.Let - : InstructionKind.Reassign; + value.lvalue.kind = InstructionKind.Const; } } else if (value.kind === "Destructure") { let kind: InstructionKind | null = null; @@ -157,7 +166,10 @@ export function leaveSSA(fn: HIRFunction): void { kind = InstructionKind.Const; } else { const originalLVal = declarations.get(place.identifier.name); - if (originalLVal === undefined) { + if ( + originalLVal === undefined || + originalLVal.lvalue === value.lvalue + ) { declarations.set(place.identifier.name, { lvalue: value.lvalue, place, @@ -207,6 +219,51 @@ export function leaveSSA(fn: HIRFunction): void { // such as for or while (and later if/switch). const reassignmentPhis: Array = []; const rewritePhis: Array = []; + function pushPhis(phiBlock: BasicBlock): void { + for (const phi of phiBlock.phis) { + if (phi.id.name === null) { + rewritePhis.push({ phi, block: phiBlock }); + } else { + reassignmentPhis.push({ phi, block: phiBlock }); + } + const hasBackEdge = backEdgePhis.has(phi); + const isPhiMutatedAfterCreation: boolean = + phi.id.mutableRange.end > + (phiBlock.instructions.at(0)?.id ?? phiBlock.terminal.id); + + // Named variables whose phi doesn't have a back-edge can potentially be independenly + // memoized, depending on whether the phi is after its creation. + if (phi.id.name !== null && !hasBackEdge) { + if (!isPhiMutatedAfterCreation) { + // Simple case: predecesor-only values flowing into a phi, which is never modified: + // adjust the phi's range to clarify that the identifier does not mutate + phi.id.mutableRange.start = terminal.id; + phi.id.mutableRange.end = makeInstructionId(terminal.id + 1); + } else { + // Predecessor only values flow into a phi, which is modified later: + // all operands flow into the phi and can be modified, must extend their ranges + for (const [, operand] of phi.operands) { + operand.mutableRange.end = phi.id.mutableRange.end; + } + } + return; + } + // Otherwise this is a temporary phi (logical or ternary) or occurs in a loop. In either + // case we can't independently memoize any of the values: unify their ranges to span the + // min(start) to max(end) so that we create a single scope for all the computation. + let start = block.terminal.id as number; + let end = Number.MIN_SAFE_INTEGER; + const operands = [phi.id, ...phi.operands.values()]; + for (const operand of operands) { + start = Math.min(start, operand.mutableRange.start); + end = Math.max(end, operand.mutableRange.end); + } + for (const operand of operands) { + operand.mutableRange.start = makeInstructionId(start); + operand.mutableRange.end = makeInstructionId(end); + } + } + } if ( (terminal.kind === "if" || terminal.kind === "switch" || @@ -215,36 +272,47 @@ export function leaveSSA(fn: HIRFunction): void { terminal.fallthrough !== null ) { const fallthrough = fn.body.blocks.get(terminal.fallthrough)!; - for (const phi of fallthrough.phis) { - if (phi.id.name == null) { - rewritePhis.push({ phi, block: fallthrough }); - } else { - reassignmentPhis.push({ phi, block: fallthrough }); - } - } + pushPhis(fallthrough); } if (terminal.kind === "while" || terminal.kind === "for") { const test = fn.body.blocks.get(terminal.test)!; - pushPhis(rewritePhis, test); - test.phis.clear(); + pushPhis(test); const loop = fn.body.blocks.get(terminal.loop)!; - pushPhis(rewritePhis, loop); - loop.phis.clear(); + pushPhis(loop); } if (terminal.kind === "for") { const init = fn.body.blocks.get(terminal.init)!; - pushPhis(rewritePhis, init); - init.phis.clear(); + pushPhis(init); + + // To avoid generating a let binding for the initializer prior to the loop, + // check to see if the for declares an iterator variable + const initIdentifier = init.instructions.at(-1); + if ( + initIdentifier !== undefined && + initIdentifier.value.kind === "StoreLocal" + ) { + const value = initIdentifier.value; + if (value.lvalue.place.identifier.name !== null) { + const originalLVal = declarations.get( + value.lvalue.place.identifier.name + ); + if (originalLVal === undefined) { + declarations.set(value.lvalue.place.identifier.name, { + lvalue: value.lvalue, + place: value.lvalue.place, + }); + value.lvalue.kind = InstructionKind.Const; + } + } + } const update = fn.body.blocks.get(terminal.update)!; - pushPhis(rewritePhis, update); - update.phis.clear(); + pushPhis(update); } if (terminal.kind === "logical" || terminal.kind === "ternary") { const fallthrough = fn.body.blocks.get(terminal.fallthrough)!; - pushPhis(rewritePhis, fallthrough); - fallthrough.phis.clear(); + pushPhis(fallthrough); } for (const { phi, block: phiBlock } of reassignmentPhis) { @@ -267,12 +335,6 @@ export function leaveSSA(fn: HIRFunction): void { phi.id.mutableRange.end > (phiBlock.instructions.at(0)?.id ?? phiBlock.terminal.id); - // If a phi is never mutated after creation, reset its mutable range to be itself - if (!isPhiMutatedAfterCreation) { - phi.id.mutableRange.start = terminal.id; - phi.id.mutableRange.end = makeInstructionId(terminal.id + 1); - } - // If we never saw a declaration for this phi, it may have been pruned by DCE, so synthesize // a new Let binding invariant( @@ -361,9 +423,6 @@ export function leaveSSA(fn: HIRFunction): void { block.instructions.push(instr); declarations.set(phi.id.name, { lvalue, place: lvalue.place }); phi.id.mutableRange.start = terminal.id; - if (!isPhiMutatedAfterCreation) { - phi.id.mutableRange.end = makeInstructionId(terminal.id + 1); - } } else if (isPhiMutatedAfterCreation) { // The declaration is not guaranteed to flow into the phi, for example in the case of a variable // 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 { canonicalId = canonicalOperand; } } - canonicalId.mutableRange.start = Math.min( - canonicalId.mutableRange.start, - terminal.id - ) as InstructionId; rewrites.set(phi.id, canonicalId); if (canonicalId.name !== null) { @@ -404,17 +459,9 @@ export function leaveSSA(fn: HIRFunction): void { } // all versions of the variable need to be remapped to the canonical id - // also extend the mutable range of the canonical id based on the min/max - // of the ranges of its operands - let start = canonicalId.mutableRange.start as number; - let end = canonicalId.mutableRange.end as number; for (const [, operand] of phi.operands) { - start = Math.min(start, operand.mutableRange.start); - end = Math.max(end, operand.mutableRange.end); rewrites.set(operand, canonicalId); } - canonicalId.mutableRange.start = makeInstructionId(start); - canonicalId.mutableRange.end = makeInstructionId(end); } } } @@ -432,20 +479,11 @@ function rewritePlace( if (nextIdentifier !== undefined) { if (nextIdentifier === prevIdentifier) return; - nextIdentifier.mutableRange.start = makeInstructionId( - Math.min( - nextIdentifier.mutableRange.start, - prevIdentifier.mutableRange.start - ) - ); - nextIdentifier.mutableRange.end = makeInstructionId( - Math.max(nextIdentifier.mutableRange.end, prevIdentifier.mutableRange.end) - ); place.identifier = nextIdentifier; } else if (prevIdentifier.name != null) { const declaration = declarations.get(prevIdentifier.name); - if (declaration === undefined) return; // Only rewrite identifiers that were declared within the function + if (declaration === undefined) return; const originalIdentifier = declaration.place.identifier; prevIdentifier.id = originalIdentifier.id; }