From 7813cfa52cf4d2d298e755699698f30199eaac5c Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Wed, 8 Mar 2023 21:10:07 -0800
Subject: [PATCH] Fix EliminateRedundantPhi for cascading eliminated phis

This was the actual bug. When EliminateRedundantPhis eliminates a phi, it has to
rewrite downstream usages of the phi id to the single operand id. We were
correctly doing that in all but one place. When we iterate _downstream phis_, we
were looking up the operands against the rewrite table, but not updating the phi
operands themselves to the rewritten value.

This fixes the bug, and incidentally fixes a test that has been broken for a
while and nagging at me.
---
 .../forget/src/SSA/EliminateRedundantPhi.ts   | 15 ++++++++---
 .../fixtures/hir/for-logical.expect.md        | 25 +++++++++++--------
 ...> ssa-cascading-eliminated-phis.expect.md} |  4 +--
 ...sa.js => ssa-cascading-eliminated-phis.js} |  0
 4 files changed, 27 insertions(+), 17 deletions(-)
 rename compiler/forget/src/__tests__/fixtures/hir/{_bug.ssa.expect.md => ssa-cascading-eliminated-phis.expect.md} (95%)
 rename compiler/forget/src/__tests__/fixtures/hir/{_bug.ssa.js => ssa-cascading-eliminated-phis.js} (100%)

diff --git a/compiler/forget/src/SSA/EliminateRedundantPhi.ts b/compiler/forget/src/SSA/EliminateRedundantPhi.ts
index cff7d3e7a6..48b3964b5f 100644
--- a/compiler/forget/src/SSA/EliminateRedundantPhi.ts
+++ b/compiler/forget/src/SSA/EliminateRedundantPhi.ts
@@ -57,12 +57,19 @@ export function eliminateRedundantPhi(fn: HIRFunction) {
 
       // Find any redundant phis
       phis: for (const phi of block.phis) {
+        // Remap phis in case operands are from eliminated phis
+        phi.operands = new Map(
+          Array.from(phi.operands).map(([block, id]) => [
+            block,
+            rewrites.get(id) ?? id,
+          ])
+        );
+        // Find if the phi can be eliminated
         let same: Identifier | null = null;
         for (const [_, operand] of phi.operands) {
-          const ident = rewrites.get(operand) ?? operand;
           if (
-            (same !== null && ident.id === same.id) ||
-            ident.id === phi.id.id
+            (same !== null && operand.id === same.id) ||
+            operand.id === phi.id.id
           ) {
             // This operand is the same as the phi or is the same as the
             // previous non-phi operands
@@ -73,7 +80,7 @@ export function eliminateRedundantPhi(fn: HIRFunction) {
             continue phis;
           } else {
             // First non-phi operand
-            same = ident;
+            same = operand;
           }
         }
         invariant(same !== null, "Expected phis to be non-empty");
diff --git a/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md b/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md
index 8a47de3f23..e465c701e6 100644
--- a/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md
+++ b/compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md
@@ -21,20 +21,23 @@ function foo(props) {
 
 ```javascript
 function foo(props) {
-  const $ = React.unstable_useMemoCache(1);
+  const $ = React.unstable_useMemoCache(2);
+  const c_0 = $[0] !== props;
   let y;
-  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+  if (c_0) {
     y = 0;
-    $[0] = y;
+    for (
+      let x = 0;
+      x > props.min && x < props.max;
+      x = x + (props.cond ? props.increment : 2), x
+    ) {
+      x = x * 2;
+      y = y + x;
+    }
+    $[0] = props;
+    $[1] = y;
   } else {
-    y = $[0];
-  }
-  for (
-    let x = 0;
-    x > props.min && x < props.max;
-    x = x + (props.cond ? props.increment : 2), x
-  ) {
-    x = x * 2;
+    y = $[1];
   }
   return y;
 }
diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug.ssa.expect.md b/compiler/forget/src/__tests__/fixtures/hir/ssa-cascading-eliminated-phis.expect.md
similarity index 95%
rename from compiler/forget/src/__tests__/fixtures/hir/_bug.ssa.expect.md
rename to compiler/forget/src/__tests__/fixtures/hir/ssa-cascading-eliminated-phis.expect.md
index 5dd7d9f93d..88538e4925 100644
--- a/compiler/forget/src/__tests__/fixtures/hir/_bug.ssa.expect.md
+++ b/compiler/forget/src/__tests__/fixtures/hir/ssa-cascading-eliminated-phis.expect.md
@@ -25,6 +25,7 @@ function Component(props) {
 ```javascript
 function Component(props) {
   const $ = React.unstable_useMemoCache(4);
+  let x = 0;
   const c_0 = $[0] !== props;
   let values;
   if (c_0) {
@@ -40,12 +41,11 @@ function Component(props) {
     }
     const y = t0;
     values.push(y);
-    let x$0 = x;
     if (props.c) {
       x = 1;
     }
 
-    values.push(x$0);
+    values.push(x);
     if (props.d) {
       x = 2;
     }
diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug.ssa.js b/compiler/forget/src/__tests__/fixtures/hir/ssa-cascading-eliminated-phis.js
similarity index 100%
rename from compiler/forget/src/__tests__/fixtures/hir/_bug.ssa.js
rename to compiler/forget/src/__tests__/fixtures/hir/ssa-cascading-eliminated-phis.js