From f9ecc96bf3a16d3a50d4a39b16af5174026ced5b Mon Sep 17 00:00:00 2001 From: Sathya Gunasekaran Date: Wed, 11 Jan 2023 15:17:14 +0000 Subject: [PATCH] [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 --- compiler/forget/src/SSA/LeaveSSA.ts | 41 ++++++++--- .../obj-literal-cached-in-if-else.expect.md | 73 +++++++++++++++++++ .../hir/obj-literal-cached-in-if-else.js | 10 +++ ...bj-literal-mutated-after-if-else.expect.md | 55 ++++++++++++++ .../hir/obj-literal-mutated-after-if-else.js | 11 +++ .../hir/obj-mutated-after-if-else.expect.md | 20 +---- 6 files changed, 183 insertions(+), 27 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/obj-literal-cached-in-if-else.js create mode 100644 compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/obj-literal-mutated-after-if-else.js 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; }