From 00d4a69459ce57afe10b24331d10e8594e09378d Mon Sep 17 00:00:00 2001 From: Sathya Gunasekaran Date: Mon, 17 Apr 2023 20:59:35 +0100 Subject: [PATCH] [hir] Refactor useMemo inlining Previously useMemo inlining created a new StoreLocal assignment (not reassignment!) instruction for every return value. This breaks when the return is inside a block (like an if-block) as the scope is tied to the block. For example: ``` let x = useMemo(() => { if (...) { return { ... }; } }) ``` would become: ``` if (...) { const temp = { ... }; } const x = temp; ``` This PR instead changes the inlining to declare a temporary in the function prologue and then reassign values to it when replacing return statements. ``` let x = useMemo(() => { if (...) { return { ... }; } }) ``` becomes ``` let temp; if (...) { temp = { ... }; } const x = temp; ``` --- .../forget/src/Inference/InlineUseMemo.ts | 55 ++++++++++++++++++- .../useMemo-if-else-multiple-return.expect.md | 29 ++++------ ...seMemo-independently-memoizeable.expect.md | 4 +- .../useMemo-inlining-block-return.expect.md | 13 ++--- ...d-statement-unconditional-return.expect.md | 4 +- .../compiler/useMemo-logical.expect.md | 4 +- .../useMemo-multiple-if-else.expect.md | 13 +++-- .../compiler/useMemo-simple.expect.md | 4 +- .../useMemo-switch-no-fallthrough.expect.md | 7 ++- 9 files changed, 87 insertions(+), 46 deletions(-) diff --git a/compiler/forget/src/Inference/InlineUseMemo.ts b/compiler/forget/src/Inference/InlineUseMemo.ts index cc09373d1c..2ac3e7a242 100644 --- a/compiler/forget/src/Inference/InlineUseMemo.ts +++ b/compiler/forget/src/Inference/InlineUseMemo.ts @@ -12,11 +12,13 @@ import { Effect, Environment, FunctionExpression, + GeneratedSource, GotoTerminal, GotoVariant, HIR, HIRFunction, IdentifierId, + Identifier, InstructionKind, makeInstructionId, makeType, @@ -198,11 +200,20 @@ export function inlineUseMemo(fn: HIRFunction): void { } } + // We store the result in the useMemo temporary + const result = instr.lvalue; + + // Declare the useMemo temporary + declareTemporary(fn.env, block, result); + + // Promote the temporary with a name as we require this to persist + promoteTemporary(result.identifier); + // Rewrite blocks from the lambda to replace any `return` with a - // store the useMemo temporary and `goto` the continuation block + // store to the result and `goto` the continuation block for (const [id, block] of body.loweredFunc.body.blocks) { block.preds.clear(); - rewriteBlock(fn.env, block, continuationBlockId, instr.lvalue); + rewriteBlock(fn.env, block, continuationBlockId, result); fn.body.blocks.set(id, block); } @@ -342,7 +353,7 @@ function rewriteBlock( }, value: { kind: "StoreLocal", - lvalue: { kind: InstructionKind.Const, place: { ...returnValue } }, + lvalue: { kind: InstructionKind.Reassign, place: { ...returnValue } }, value: terminal.value, loc: terminal.loc, }, @@ -356,3 +367,41 @@ function rewriteBlock( loc: block.terminal.loc, }; } + +function declareTemporary( + env: Environment, + block: BasicBlock, + result: Place +): void { + block.instructions.push({ + id: makeInstructionId(0), + loc: GeneratedSource, + lvalue: { + effect: Effect.Unknown, + identifier: { + id: env.nextIdentifierId, + mutableRange: { + start: makeInstructionId(0), + end: makeInstructionId(0), + }, + name: null, + scope: null, + type: makeType(), + }, + kind: "Identifier", + loc: GeneratedSource, + }, + value: { + kind: "DeclareLocal", + lvalue: { + place: result, + kind: InstructionKind.Let, + }, + loc: result.loc, + }, + }); +} + +function promoteTemporary(temp: Identifier): void { + temp.name = `t${temp.id}`; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md index a2abf46ae4..8175b3419b 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-if-else-multiple-return.expect.md @@ -20,7 +20,8 @@ function Component(props) { ```javascript // @inlineUseMemo function Component(props) { - const $ = React.unstable_useMemoCache(5); + const $ = React.unstable_useMemoCache(4); + let t16 = undefined; if (props.cond) { const c_0 = $[0] !== props.a; let t0; @@ -31,26 +32,20 @@ function Component(props) { } else { t0 = $[1]; } - let t1; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t1 = t0; - $[2] = t1; - } else { - t1 = $[2]; - } + t16 = t0; } else { - const c_3 = $[3] !== props.b; - let t2; - if (c_3) { - t2 = makeObject(props.b); - $[3] = props.b; - $[4] = t2; + const c_2 = $[2] !== props.b; + let t1; + if (c_2) { + t1 = makeObject(props.b); + $[2] = props.b; + $[3] = t1; } else { - t2 = $[4]; + t1 = $[3]; } - t1 = t2; + t16 = t1; } - const x = t1; + const x = t16; return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-independently-memoizeable.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-independently-memoizeable.expect.md index 32767bb99b..f438ea86a6 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-independently-memoizeable.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-independently-memoizeable.expect.md @@ -52,8 +52,8 @@ function Component(props) { } else { t2 = $[6]; } - const t54 = t2; - const [a_0, b_0] = t54; + const t24 = t2; + const [a_0, b_0] = t24; const c_7 = $[7] !== a_0; const c_8 = $[8] !== b_0; let t3; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md index ac194836d7..09bfe4c33a 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-inlining-block-return.expect.md @@ -19,7 +19,8 @@ function component(a, b) { ```javascript // @inlineUseMemo function component(a, b) { - const $ = React.unstable_useMemoCache(3); + const $ = React.unstable_useMemoCache(2); + let t13; if (a) { const c_0 = $[0] !== b; let t0; @@ -30,15 +31,9 @@ function component(a, b) { } else { t0 = $[1]; } - let t1; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t1 = t0; - $[2] = t1; - } else { - t1 = $[2]; - } + t13 = t0; } - const x = t1; + const x = t13; return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-labeled-statement-unconditional-return.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-labeled-statement-unconditional-return.expect.md index 5821491900..43a5fef4f0 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-labeled-statement-unconditional-return.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-labeled-statement-unconditional-return.expect.md @@ -19,8 +19,8 @@ function Component(props) { ```javascript // @inlineUseMemo function Component(props) { - const t19 = props.value; - const x = t19; + const t8 = props.value; + const x = t8; return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-logical.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-logical.expect.md index 9542e3f2fd..f332d4d1fb 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-logical.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-logical.expect.md @@ -15,8 +15,8 @@ function Component(props) { ```javascript // @inlineUseMemo function Component(props) { - const t32 = props.a && props.b; - const x = t32; + const t14 = props.a && props.b; + const x = t14; return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md index a99608301f..c13d033e2d 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-multiple-if-else.expect.md @@ -27,24 +27,25 @@ function Component(props) { function Component(props) { const $ = React.unstable_useMemoCache(2); const c_0 = $[0] !== props; - let t0; + let t25; if (c_0) { const y = []; if (props.cond) { y.push(props.a); } + t25 = undefined; if (props.cond2) { - t0 = y; + t25 = y; } else { y.push(props.b); - t0 = y; + t25 = y; } $[0] = props; - $[1] = t0; + $[1] = t25; } else { - t0 = $[1]; + t25 = $[1]; } - const x = t0; + const x = t25; return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md index 190d780bfb..2e7e4dea14 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md @@ -25,8 +25,8 @@ function component(a) { } else { t0 = $[1]; } - const t23 = t0; - const x = t23; + const t9 = t0; + const x = t9; const c_2 = $[2] !== x; let t1; if (c_2) { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-switch-no-fallthrough.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-switch-no-fallthrough.expect.md index 836c12739b..b44ff2ec64 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-switch-no-fallthrough.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-switch-no-fallthrough.expect.md @@ -24,16 +24,17 @@ function Component(props) { ```javascript // @inlineUseMemo function Component(props) { + let t13 = undefined; bb8: switch (props.key) { case "key": { - const t28 = props.value; + t13 = props.value; break bb8; } default: { - const t28 = props.defaultValue; + t13 = props.defaultValue; } } - const x = t28; + const x = t13; return x; }