From 832285fed2aaaff58001539a5eebe6d580cb280a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 1 Apr 2024 14:21:48 -0700 Subject: [PATCH] Only create mutable ranges for phis *mutated* later This PR was the result of a long chain of ~yak-shaving~ debugging kicked off as a result of fixing up invariants. Where this started was that i noticed some cases of loops where the first instance we saw of a reactive scope was after its starting instruction. Eg instruction N would have an operand with scope Start:End, where Start was _before_ N. One of the cases involved a phi with a backedge. Then i noticed that we assign scopes differently for phis with and without backedges: ``` [1] let x0 = init; [2] if (x0 < limit) { [3] x1 += increment; } x2 = phi(x0, x1); [4] x2; ``` The phi isn't mutated _or_ reassigned after its creation, so we don't assign a mutable range to the phi or any of its operands. We also don't create a scope for `x`. But change the `if` to a `while` and now the phi moves - now there's a backedge: ``` [1] let x0 = init; [2] while (x0 < limit) { x2 = phi(x0, x2); // now this is "mutated" later!!! [3] x2 += increment; } [4] x2; ``` What was happening here is that x2 has a mutable range which is "after" the phi instruction, so it would appear that the phi was actually being mutated later. Ie, this was treated equivalently to the original "if" version, but with a mutation: ``` let x = []; if (cond) { x = {}; } mutate(x); // later mutation of the phi ``` But these latter two cases are different! We only need to (should) create a mutable range for a phi _if its value is actually mutated_. If it's just being reassigned, well then it shouldn't matter if there are back edges or not. So this PR implements that intuition: only create a mutable range for a phi if it is actually _mutated_ later, ie don't assign a mutable range if it is only _reassigned_ later. Concretely in InferMutableRanges: * InferMutableLifetimes no longer has to initialize a range for phis during the first pass (inferMutableRangesForStores=false). We wait to see if the phi is mutated during the main fixpoint iteration of InferMutableRanges * The main fixpoint iteration in InferMutableRanges already aliases phi operands if the phi is later mutated, which will extend the end of the mutable range of all the operands accordingly. * Finally, InferMutableLifetimes's second run (inferMutableRangesForStores=true), we ensure that any phis mutated later have a valid mutable range, specifically setting the `start` of the range since the fixpoint only updates the `end` value. --- .../src/Inference/InferMutableLifetimes.ts | 38 ++++++++----------- .../InferReactiveScopeVariables.ts | 3 ++ .../compiler/for-in-statement-break.expect.md | 22 ++++++----- .../for-in-statement-continue.expect.md | 22 ++++++----- .../compiler/sequence-expression.expect.md | 15 ++++---- 5 files changed, 51 insertions(+), 49 deletions(-) diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts index 20d50af1fe..dda812bb5d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts @@ -112,28 +112,22 @@ export function inferMutableLifetimes( >(); for (const [_, block] of func.body.blocks) { for (const phi of block.phis) { - for (const [_, operand] of phi.operands) { - if ( - operand.mutableRange.start === 0 && - operand.mutableRange.end === 0 - ) { - // operand's range is uninitialized, skip - continue; - } else if ( - phi.id.mutableRange.start === 0 && - phi.id.mutableRange.end === 0 - ) { - // phi's range is uninitialized, take the range from the operand - phi.id.mutableRange.start = operand.mutableRange.start; - phi.id.mutableRange.end = operand.mutableRange.end; - } else { - // else join the phi and operand's range - phi.id.mutableRange.start = makeInstructionId( - Math.min(phi.id.mutableRange.start, operand.mutableRange.start) - ); - phi.id.mutableRange.end = makeInstructionId( - Math.max(phi.id.mutableRange.end, operand.mutableRange.end) - ); + const isPhiMutatedAfterCreation: boolean = + phi.id.mutableRange.end > + (block.instructions.at(0)?.id ?? block.terminal.id); + if ( + inferMutableRangeForStores && + isPhiMutatedAfterCreation && + phi.id.mutableRange.start === 0 + ) { + for (const [, operand] of phi.operands) { + if (phi.id.mutableRange.start === 0) { + phi.id.mutableRange.start = operand.mutableRange.start; + } else { + phi.id.mutableRange.start = makeInstructionId( + Math.min(phi.id.mutableRange.start, operand.mutableRange.start) + ); + } } } } diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts index e998573b61..64c960946f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -22,6 +22,7 @@ import { eachPatternOperand, } from "../HIR/visitors"; import DisjointSet from "../Utils/DisjointSet"; +import { logHIRFunction } from "../Utils/logger"; import { assertExhaustive } from "../Utils/utils"; /* @@ -145,6 +146,8 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { maxInstruction === 0 || scope.range.end > maxInstruction + 1 ) { + // Make it easier to debug why the error occurred + logHIRFunction("InferReactiveScopeVariables (invalid scope)", fn); CompilerError.invariant(false, { reason: `Invalid mutable range for scope`, loc: GeneratedSource, diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-break.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-break.expect.md index 4127e645b5..e3a11f9768 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-break.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-break.expect.md @@ -29,19 +29,21 @@ import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { const $ = useMemoCache(2); let x; + let t0; if ($[0] !== props.value) { - const object = { ...props.value }; - for (const y in object) { - if (y === "break") { - break; - } - - x = object[y]; - } + t0 = { ...props.value }; $[0] = props.value; - $[1] = x; + $[1] = t0; } else { - x = $[1]; + t0 = $[1]; + } + const object = t0; + for (const y in object) { + if (y === "break") { + break; + } + + x = object[y]; } return x; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md index 213d414350..2a29176e6a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md @@ -38,19 +38,21 @@ import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { const $ = useMemoCache(2); let x; + let t0; if ($[0] !== props.value) { - const object = { ...props.value }; - for (const y in object) { - if (y === "continue") { - continue; - } - - x = object[y]; - } + t0 = { ...props.value }; $[0] = props.value; - $[1] = x; + $[1] = t0; } else { - x = $[1]; + t0 = $[1]; + } + const object = t0; + for (const y in object) { + if (y === "continue") { + continue; + } + + x = object[y]; } return x; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md index 7c7325627b..533927d150 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md @@ -20,15 +20,16 @@ function foo() {} import { unstable_useMemoCache as useMemoCache } from "react"; function sequence(props) { const $ = useMemoCache(1); - let x; + let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - x = (Math.max(1, 2), foo()); - while ((foo(), true)) { - x = (foo(), 2); - } - $[0] = x; + t0 = (Math.max(1, 2), foo()); + $[0] = t0; } else { - x = $[0]; + t0 = $[0]; + } + let x = t0; + while ((foo(), true)) { + x = (foo(), 2); } return x; }