From a9868cc2b1c19066e9d93bbc404fdca02a314651 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 16 Jun 2023 14:42:19 -0700 Subject: [PATCH] Fix memoization of lambdas in @enableOptimizeFunctionExpr feature This is a fix specifically for the `enableOptimizeFunctionExpression` feature (disabled by default). I tried running all of our fixtures with that flag on everywhere, and this was the only issue. It's actually extracted from another fixture which is more complicated, this is a distilled version. There were two bugs: * DCE was running after LeaveSSA when it needs to run before. Fixing the order means code inside the function expression stops getting removed. * In the feature flag, context variables share Identifier instances with the surrounding code, whereas they are distinct instances without the feature. This meant that mutable ranges from inside the function propagated outside the function, throwing off our inference. The fix was to reset the ranges of context variables after inferring the function expression's effects. --- .../src/Inference/AnalyseFunctions.ts | 4 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 2 - ...-in-returned-function-expression.expect.md | 48 +++++++++++++++++++ ...ned-phi-in-returned-function-expression.js | 12 +++++ 4 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.expect.md create mode 100644 compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.js diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts b/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts index bbfdb796cb..c40fc5fee2 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts +++ b/compiler/forget/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts @@ -17,7 +17,7 @@ import { Place, ReactiveScopeDependency, } from "../HIR"; -import { constantPropagation } from "../Optimization"; +import { constantPropagation, deadCodeElimination } from "../Optimization"; import { inferReactiveScopeVariables } from "../ReactiveScopes"; import { eliminateRedundantPhi, enterSSA, leaveSSA } from "../SSA"; import { inferTypes } from "../TypeInference"; @@ -113,6 +113,7 @@ function lower(func: HIRFunction): void { analyseFunctions(func); inferReferenceEffects(func, { isFunctionExpression: true }); + deadCodeElimination(func); inferMutableRanges(func); leaveSSA(func); inferReactiveScopeVariables(func); @@ -133,6 +134,7 @@ function infer( ) { mutations.set(operand.identifier.name, operand.effect); } + operand.identifier.mutableRange.end = operand.identifier.mutableRange.start; } for (const dep of value.dependencies) { diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts index a9a495d780..257565715a 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/forget/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -31,7 +31,6 @@ import { } from "../HIR/HIR"; import { printPlace } from "../HIR/PrintHIR"; import { eachPatternOperand } from "../HIR/visitors"; -import { deadCodeElimination } from "../Optimization"; import { Err, Ok, Result } from "../Utils/Result"; import { assertExhaustive } from "../Utils/utils"; import { buildReactiveFunction } from "./BuildReactiveFunction"; @@ -962,7 +961,6 @@ function codegenInstructionValue( case "FunctionExpression": { if (cx.env.enableOptimizeFunctionExpressions) { const loweredFunc = instrValue.loweredFunc; - deadCodeElimination(loweredFunc); const reactiveFunction = buildReactiveFunction(loweredFunc); pruneUnusedLabels(reactiveFunction); pruneUnusedLValues(reactiveFunction); diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.expect.md new file mode 100644 index 0000000000..13e811b62f --- /dev/null +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.expect.md @@ -0,0 +1,48 @@ + +## Input + +```javascript +// @enableOptimizeFunctionExpressions +function Component(props) { + return () => { + let str; + if (arguments.length) { + str = arguments[0]; + } else { + str = props.str; + } + global.log(str); + }; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; // @enableOptimizeFunctionExpressions +function Component(props) { + const $ = useMemoCache(2); + const c_0 = $[0] !== props.str; + let t0; + if (c_0) { + t0 = () => { + let str = undefined; + if (arguments.length) { + str = arguments[0]; + } else { + str = props.str; + } + + global.log(str); + }; + $[0] = props.str; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.js new file mode 100644 index 0000000000..dcf5ecc79c --- /dev/null +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reassigned-phi-in-returned-function-expression.js @@ -0,0 +1,12 @@ +// @enableOptimizeFunctionExpressions +function Component(props) { + return () => { + let str; + if (arguments.length) { + str = arguments[0]; + } else { + str = props.str; + } + global.log(str); + }; +}