@samitouri / QOS-React-2 / commits / 015029dc65

[patch] Fix for constant propagation bug in VR Store

--- Changes in `@enableOptimizeFunctionExpressions` caused a bug in the last Forget sync to VR Store. The repro can be summarized to something like this: ```js function foo() { const x = true; // some constant or global // Add some branching for type inference // This can be a Logical expression as well (e.g. `4 || 5`) if (...) { } // In this HIR block, SSA inserts a `x$2 = phi(x$1, x$1)`. // EliminateRedundantPhiNodes needs to rewrite all references of `x$2` to `x$1` const accessXInLambda = () => x; return accessXInLambda; }

Mofei Zhang committed Aug 2, 2023 at 17:30 UTC 015029dc655d0593a470bb4bcefd08e4126aaf48
4 files changed +189 -3
compiler/forget/packages/babel-plugin-react-forget/src/SSA/EliminateRedundantPhi.ts
+14 -3
@@ -27,9 +27,13 @@ import {
27 * and phis rewrite all their identifiers based on this table. The algorithm loops over the CFG repeatedly
28 * until there are no new rewrites: for a CFG without back-edges it completes in a single pass.
29 */
30 -export function eliminateRedundantPhi(fn: HIRFunction): void {
30 +export function eliminateRedundantPhi(
31 + fn: HIRFunction,
32 + sharedRewrites?: Map<Identifier, Identifier>
33 +): void {
34 const ir = fn.body;
32 - const rewrites: Map<Identifier, Identifier> = new Map();
35 + const rewrites: Map<Identifier, Identifier> =
36 + sharedRewrites != null ? sharedRewrites : new Map();
37
38 // Whether or the CFG has a back-edge (a loop). We determine this dynamically
39 // during the first iteration over the CFG by recording which blocks were already
@@ -102,6 +106,13 @@ export function eliminateRedundantPhi(fn: HIRFunction): void {
106 for (const place of eachInstructionOperand(instr)) {
107 rewritePlace(place, rewrites);
108 }
109 + if (instr.value.kind === "FunctionExpression") {
110 + const { context } = instr.value.loweredFunc;
111 + for (const place of context) {
112 + rewritePlace(place, rewrites);
113 + }
114 + }
115 +
116 rewritePlace(instr.lvalue, rewrites);
117
118 // visit function expressions on first iteration of each block
@@ -110,7 +121,7 @@ export function eliminateRedundantPhi(fn: HIRFunction): void {
121 instr.value.kind === "FunctionExpression" &&
122 fn.env.enableOptimizeFunctionExpressions
123 ) {
113 - eliminateRedundantPhi(instr.value.loweredFunc);
124 + eliminateRedundantPhi(instr.value.loweredFunc, rewrites);
125 }
126 }
127
compiler/forget/packages/babel-plugin-react-forget/src/__tests__/e2e/constant-prop.e2e.js new
+124
@@ -0,0 +1,124 @@
1 +/**
2 + * Copyright (c) Meta Platforms, Inc. and affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + */
7 +
8 +import * as React from "react";
9 +import { render } from "@testing-library/react";
10 +
11 +globalThis.constantValue = "global test value";
12 +
13 +test("literal-constant-propagation", () => {
14 + function Component() {
15 + const x = "test value 1";
16 + return <div>{x}</div>;
17 + }
18 + const { asFragment, rerender } = render(<Component />);
19 +
20 + expect(asFragment()).toMatchInlineSnapshot(`
21 + <DocumentFragment>
22 + <div>
23 + test value 1
24 + </div>
25 + </DocumentFragment>
26 + `);
27 +
28 + rerender(<Component />);
29 +
30 + expect(asFragment()).toMatchInlineSnapshot(`
31 + <DocumentFragment>
32 + <div>
33 + test value 1
34 + </div>
35 + </DocumentFragment>
36 + `);
37 +});
38 +
39 +test("global-constant-propagation", () => {
40 + function Component() {
41 + const x = constantValue;
42 +
43 + return <div>{x}</div>;
44 + }
45 + const { asFragment, rerender } = render(<Component />);
46 +
47 + expect(asFragment()).toMatchInlineSnapshot(`
48 + <DocumentFragment>
49 + <div>
50 + global test value
51 + </div>
52 + </DocumentFragment>
53 + `);
54 +
55 + rerender(<Component />);
56 +
57 + expect(asFragment()).toMatchInlineSnapshot(`
58 + <DocumentFragment>
59 + <div>
60 + global test value
61 + </div>
62 + </DocumentFragment>
63 + `);
64 +});
65 +
66 +test("lambda-constant-propagation", () => {
67 + function Component() {
68 + const x = "test value 1";
69 + const getDiv = () => <div>{x}</div>;
70 + return getDiv();
71 + }
72 + const { asFragment, rerender } = render(<Component />);
73 +
74 + expect(asFragment()).toMatchInlineSnapshot(`
75 + <DocumentFragment>
76 + <div>
77 + test value 1
78 + </div>
79 + </DocumentFragment>
80 + `);
81 +
82 + rerender(<Component />);
83 +
84 + expect(asFragment()).toMatchInlineSnapshot(`
85 + <DocumentFragment>
86 + <div>
87 + test value 1
88 + </div>
89 + </DocumentFragment>
90 + `);
91 +});
92 +
93 +test("lambda-constant-propagation-of-phi-node", () => {
94 + function Component({ noopCallback }) {
95 + const x = "test value 1";
96 + if (constantValue) {
97 + noopCallback();
98 + }
99 + const getDiv = () => <div>{x}</div>;
100 + return getDiv();
101 + }
102 +
103 + const { asFragment, rerender } = render(
104 + <Component noopCallback={() => {}} />
105 + );
106 +
107 + expect(asFragment()).toMatchInlineSnapshot(`
108 + <DocumentFragment>
109 + <div>
110 + test value 1
111 + </div>
112 + </DocumentFragment>
113 + `);
114 +
115 + rerender(<Component noopCallback={() => {}} />);
116 +
117 + expect(asFragment()).toMatchInlineSnapshot(`
118 + <DocumentFragment>
119 + <div>
120 + test value 1
121 + </div>
122 + </DocumentFragment>
123 + `);
124 +});
compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rewrite-phis-in-lambda-capture-context.expect.md new
+43
@@ -0,0 +1,43 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function ConstantPropagationBug() {
6 + const x = CONSTANT1;
7 + const createPhiNode = CONSTANT2 || 5;
8 +
9 + const getFoo = () => <Foo x={x} y={createPhiNode} />;
10 +
11 + return getFoo();
12 +}
13 +
14 +```
15 +
16 +## Code
17 +
18 +```javascript
19 +import { unstable_useMemoCache as useMemoCache } from "react";
20 +function ConstantPropagationBug() {
21 + const $ = useMemoCache(2);
22 +
23 + const createPhiNode = CONSTANT2 || 5;
24 + let t0;
25 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
26 + t0 = () => <Foo x={CONSTANT1} y={createPhiNode} />;
27 + $[0] = t0;
28 + } else {
29 + t0 = $[0];
30 + }
31 + const getFoo = t0;
32 + let t1;
33 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
34 + t1 = getFoo();
35 + $[1] = t1;
36 + } else {
37 + t1 = $[1];
38 + }
39 + return t1;
40 +}
41 +
42 +```
43 +
\ No newline at end of file
compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/rewrite-phis-in-lambda-capture-context.js new
+8
@@ -0,0 +1,8 @@
1 +function ConstantPropagationBug() {
2 + const x = CONSTANT1;
3 + const createPhiNode = CONSTANT2 || 5;
4 +
5 + const getFoo = () => <Foo x={x} y={createPhiNode} />;
6 +
7 + return getFoo();
8 +}