@samitouri / QOS-React-1 / commits / 832285fed2

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.

Joe Savona committed Apr 1, 2024 at 14:21 UTC 832285fed2aaaff58001539a5eebe6d580cb280a
5 files changed +51 -49
compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts
+16 -22
@@ -112,28 +112,22 @@ export function inferMutableLifetimes(
112 >();
113 for (const [_, block] of func.body.blocks) {
114 for (const phi of block.phis) {
115 - for (const [_, operand] of phi.operands) {
116 - if (
117 - operand.mutableRange.start === 0 &&
118 - operand.mutableRange.end === 0
119 - ) {
120 - // operand's range is uninitialized, skip
121 - continue;
122 - } else if (
123 - phi.id.mutableRange.start === 0 &&
124 - phi.id.mutableRange.end === 0
125 - ) {
126 - // phi's range is uninitialized, take the range from the operand
127 - phi.id.mutableRange.start = operand.mutableRange.start;
128 - phi.id.mutableRange.end = operand.mutableRange.end;
129 - } else {
130 - // else join the phi and operand's range
131 - phi.id.mutableRange.start = makeInstructionId(
132 - Math.min(phi.id.mutableRange.start, operand.mutableRange.start)
133 - );
134 - phi.id.mutableRange.end = makeInstructionId(
135 - Math.max(phi.id.mutableRange.end, operand.mutableRange.end)
136 - );
115 + const isPhiMutatedAfterCreation: boolean =
116 + phi.id.mutableRange.end >
117 + (block.instructions.at(0)?.id ?? block.terminal.id);
118 + if (
119 + inferMutableRangeForStores &&
120 + isPhiMutatedAfterCreation &&
121 + phi.id.mutableRange.start === 0
122 + ) {
123 + for (const [, operand] of phi.operands) {
124 + if (phi.id.mutableRange.start === 0) {
125 + phi.id.mutableRange.start = operand.mutableRange.start;
126 + } else {
127 + phi.id.mutableRange.start = makeInstructionId(
128 + Math.min(phi.id.mutableRange.start, operand.mutableRange.start)
129 + );
130 + }
131 }
132 }
133 }
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts
+3
@@ -22,6 +22,7 @@ import {
22 eachPatternOperand,
23 } from "../HIR/visitors";
24 import DisjointSet from "../Utils/DisjointSet";
25 +import { logHIRFunction } from "../Utils/logger";
26 import { assertExhaustive } from "../Utils/utils";
27
28 /*
@@ -145,6 +146,8 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
146 maxInstruction === 0 ||
147 scope.range.end > maxInstruction + 1
148 ) {
149 + // Make it easier to debug why the error occurred
150 + logHIRFunction("InferReactiveScopeVariables (invalid scope)", fn);
151 CompilerError.invariant(false, {
152 reason: `Invalid mutable range for scope`,
153 loc: GeneratedSource,
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-break.expect.md
+12 -10
@@ -29,19 +29,21 @@ import { unstable_useMemoCache as useMemoCache } from "react";
29 function Component(props) {
30 const $ = useMemoCache(2);
31 let x;
32 + let t0;
33 if ($[0] !== props.value) {
33 - const object = { ...props.value };
34 - for (const y in object) {
35 - if (y === "break") {
36 - break;
37 - }
38 -
39 - x = object[y];
40 - }
34 + t0 = { ...props.value };
35 $[0] = props.value;
42 - $[1] = x;
36 + $[1] = t0;
37 } else {
44 - x = $[1];
38 + t0 = $[1];
39 + }
40 + const object = t0;
41 + for (const y in object) {
42 + if (y === "break") {
43 + break;
44 + }
45 +
46 + x = object[y];
47 }
48 return x;
49 }
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md
+12 -10
@@ -38,19 +38,21 @@ import { unstable_useMemoCache as useMemoCache } from "react";
38 function Component(props) {
39 const $ = useMemoCache(2);
40 let x;
41 + let t0;
42 if ($[0] !== props.value) {
42 - const object = { ...props.value };
43 - for (const y in object) {
44 - if (y === "continue") {
45 - continue;
46 - }
47 -
48 - x = object[y];
49 - }
43 + t0 = { ...props.value };
44 $[0] = props.value;
51 - $[1] = x;
45 + $[1] = t0;
46 } else {
53 - x = $[1];
47 + t0 = $[1];
48 + }
49 + const object = t0;
50 + for (const y in object) {
51 + if (y === "continue") {
52 + continue;
53 + }
54 +
55 + x = object[y];
56 }
57 return x;
58 }
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequence-expression.expect.md
+8 -7
@@ -20,15 +20,16 @@ function foo() {}
20 import { unstable_useMemoCache as useMemoCache } from "react";
21 function sequence(props) {
22 const $ = useMemoCache(1);
23 - let x;
23 + let t0;
24 if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
25 - x = (Math.max(1, 2), foo());
26 - while ((foo(), true)) {
27 - x = (foo(), 2);
28 - }
29 - $[0] = x;
25 + t0 = (Math.max(1, 2), foo());
26 + $[0] = t0;
27 } else {
31 - x = $[0];
28 + t0 = $[0];
29 + }
30 + let x = t0;
31 + while ((foo(), true)) {
32 + x = (foo(), 2);
33 }
34 return x;
35 }