diff --git a/compiler/forget/src/SSA/LeaveSSA.ts b/compiler/forget/src/SSA/LeaveSSA.ts index f3998b5439..ea2a65ef84 100644 --- a/compiler/forget/src/SSA/LeaveSSA.ts +++ b/compiler/forget/src/SSA/LeaveSSA.ts @@ -7,6 +7,8 @@ import invariant from "invariant"; import { + BasicBlock, + BlockId, Effect, GeneratedSource, HIRFunction, @@ -96,6 +98,16 @@ export function leaveSSA(fn: HIRFunction) { // phi id or, more typically, the operand that was defined prior to the phi. const rewrites: Map = new Map(); + type PhiState = { + phi: Phi; + block: BasicBlock; + }; + function pushPhis(arr: Array, block: BasicBlock) { + for (const phi of block.phis) { + arr.push({ phi, block }); + } + } + for (const [, block] of fn.body.blocks) { invariant( block.phis.size === 0, @@ -105,8 +117,8 @@ export function leaveSSA(fn: HIRFunction) { // Find any phi nodes which need a variable declaration in the current block // This includes phis in fallthrough nodes, or blocks that form part of control flow // such as for or while (and later if/switch). - const reassignmentPhis: Array = []; - const rewritePhis: Array = []; + const reassignmentPhis: Array = []; + const rewritePhis: Array = []; const terminal = block.terminal; if ( (terminal.kind === "if" || @@ -116,29 +128,29 @@ export function leaveSSA(fn: HIRFunction) { terminal.fallthrough !== null ) { const fallthrough = fn.body.blocks.get(terminal.fallthrough)!; - reassignmentPhis.push(...fallthrough.phis); + pushPhis(reassignmentPhis, fallthrough); fallthrough.phis.clear(); } if (terminal.kind === "while" || terminal.kind === "for") { const test = fn.body.blocks.get(terminal.test)!; - rewritePhis.push(...test.phis); + pushPhis(rewritePhis, test); test.phis.clear(); const loop = fn.body.blocks.get(terminal.loop)!; - rewritePhis.push(...loop.phis); + pushPhis(rewritePhis, loop); loop.phis.clear(); } if (terminal.kind === "for") { const init = fn.body.blocks.get(terminal.init)!; - rewritePhis.push(...init.phis); + pushPhis(rewritePhis, init); init.phis.clear(); const update = fn.body.blocks.get(terminal.update)!; - rewritePhis.push(...update.phis); + pushPhis(rewritePhis, update); update.phis.clear(); } - for (const phi of reassignmentPhis) { + for (const { phi, block: phiBlock } of reassignmentPhis) { // In some cases one of the phi operands can be defined *before* the let binding // we will generate. For example, a variable that is only rebound in one branch of // 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) { canonicalId.mutableRange.start = makeInstructionId(start); canonicalId.mutableRange.end = makeInstructionId(end); + // If there are no instructions in the block then there's just a terminal + // node, which has no mutation, so that should be false. + // + // TODO(joe): This above statement is true, right? Could there be a value + // block with instructions in terminals? + const isPhiMutatedAfterCreation: boolean = + end > (phiBlock.instructions.at(0)?.id ?? end); + // If this phi id is the canonical id we need to generate a let binding for it // (otherwise, it means this phi merges into some other phi which already generated // a binding @@ -220,6 +240,9 @@ export function leaveSSA(fn: HIRFunction) { if (operand === initOperand) { continue; } + if (isPhiMutatedAfterCreation) { + operand.mutableRange.end = canonicalId.mutableRange.end; + } const predecessor = fn.body.blocks.get(predecessorId)!; const instr: Instruction = { id: predecessor.terminal.id, @@ -247,7 +270,7 @@ export function leaveSSA(fn: HIRFunction) { // Similar logic for rewrite phis that occur in loops, except that instead of a new let binding // we pick one of the operands as the canonical id, and rewrite all references to the other // operands and the phi to reference this canonical id. - for (const phi of rewritePhis) { + for (const { phi } of rewritePhis) { let canonicalId = rewrites.get(phi.id); if (canonicalId === undefined) { canonicalId = phi.id; diff --git a/compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.expect.md b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.expect.md new file mode 100644 index 0000000000..de9a4db5de --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +function foo(a, b, c, d) { + let x = {}; + if (someVal) { + x = { b }; + } else { + x = { c }; + } + + return x; +} + +``` + +## Code + +```javascript +function foo(a, b, c, d) { + const $ = React.useMemoCache(); + const x = {}; + const c_0 = $[0] !== b; + const c_1 = $[1] !== c; + let x$0; + if (c_0 || c_1) { + x$0 = undefined; + + if (someVal) { + const c_3 = $[3] !== b; + let x$1; + + if (c_3) { + x$1 = { + b: b, + }; + $[3] = b; + $[4] = x$1; + } else { + x$1 = $[4]; + } + + x$0 = x$1; + } else { + const c_5 = $[5] !== c; + let x$2; + + if (c_5) { + x$2 = { + c: c, + }; + $[5] = c; + $[6] = x$2; + } else { + x$2 = $[6]; + } + + x$0 = x$2; + } + + $[0] = b; + $[1] = c; + $[2] = x$0; + } else { + x$0 = $[2]; + } + + return x$0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.js b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.js new file mode 100644 index 0000000000..2ccd25119b --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.js @@ -0,0 +1,10 @@ +function foo(a, b, c, d) { + let x = {}; + if (someVal) { + x = { b }; + } else { + x = { c }; + } + + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.expect.md b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.expect.md new file mode 100644 index 0000000000..a3df518a3f --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.expect.md @@ -0,0 +1,55 @@ + +## Input + +```javascript +function foo(a, b, c, d) { + let x = {}; + if (someVal) { + x = { b }; + } else { + x = { c }; + } + + x.f = 1; + return x; +} + +``` + +## Code + +```javascript +function foo(a, b, c, d) { + const $ = React.useMemoCache(); + const x = {}; + const c_0 = $[0] !== b; + const c_1 = $[1] !== c; + let x$0; + if (c_0 || c_1) { + x$0 = undefined; + + if (someVal) { + const x$1 = { + b: b, + }; + x$0 = x$1; + } else { + const x$2 = { + c: c, + }; + x$0 = x$2; + } + + x$0.f = 1; + $[0] = b; + $[1] = c; + $[2] = x$0; + } else { + x$0 = $[2]; + } + + return x$0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.js b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.js new file mode 100644 index 0000000000..4ed2c5b6fb --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.js @@ -0,0 +1,11 @@ +function foo(a, b, c, d) { + let x = {}; + if (someVal) { + x = { b }; + } else { + x = { c }; + } + + x.f = 1; + return x; +} diff --git a/compiler/forget/src/__tests__/fixtures/hir/obj-mutated-after-if-else.expect.md b/compiler/forget/src/__tests__/fixtures/hir/obj-mutated-after-if-else.expect.md index a963142635..d1aaa70beb 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/obj-mutated-after-if-else.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/obj-mutated-after-if-else.expect.md @@ -28,26 +28,10 @@ function foo(a, b, c, d) { x$0 = undefined; if (a) { - let x$1; - - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - x$1 = someObj(); - $[2] = x$1; - } else { - x$1 = $[2]; - } - + const x$1 = someObj(); x$0 = x$1; } else { - let x$2; - - if ($[3] === Symbol.for("react.memo_cache_sentinel")) { - x$2 = someObj(); - $[3] = x$2; - } else { - x$2 = $[3]; - } - + const x$2 = someObj(); x$0 = x$2; }