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.
Joe Savona committed
Mar 8, 2023 at 21:10 UTC
7813cfa52cf4d2d298e755699698f30199eaac5c
4 files changed
+27
-17
compiler/forget/src/SSA/EliminateRedundantPhi.ts
+11
-4
@@ -57,12 +57,19 @@ export function eliminateRedundantPhi(fn: HIRFunction) {
57
58
// Find any redundant phis
59
phis: for (const phi of block.phis) {
60
+ // Remap phis in case operands are from eliminated phis
61
+ phi.operands = new Map(
62
+ Array.from(phi.operands).map(([block, id]) => [
63
+ block,
64
+ rewrites.get(id) ?? id,
65
+ ])
66
+ );
67
+ // Find if the phi can be eliminated
68
let same: Identifier | null = null;
69
for (const [_, operand] of phi.operands) {
62
- const ident = rewrites.get(operand) ?? operand;
70
if (
64
- (same !== null && ident.id === same.id) ||
65
- ident.id === phi.id.id
71
+ (same !== null && operand.id === same.id) ||
72
+ operand.id === phi.id.id
73
) {
74
// This operand is the same as the phi or is the same as the
75
// previous non-phi operands
@@ -73,7 +80,7 @@ export function eliminateRedundantPhi(fn: HIRFunction) {
80
continue phis;
81
} else {
82
// First non-phi operand
76
- same = ident;
83
+ same = operand;
84
}
85
}
86
invariant(same !== null, "Expected phis to be non-empty");
compiler/forget/src/__tests__/fixtures/hir/for-logical.expect.md
+14
-11
@@ -21,20 +21,23 @@ function foo(props) {
21
22
```javascript
23
function foo(props) {
24
- const $ = React.unstable_useMemoCache(1);
24
+ const $ = React.unstable_useMemoCache(2);
25
+ const c_0 = $[0] !== props;
26
let y;
26
- if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
27
+ if (c_0) {
28
y = 0;
28
- $[0] = y;
29
+ for (
30
+ let x = 0;
31
+ x > props.min && x < props.max;
32
+ x = x + (props.cond ? props.increment : 2), x
33
+ ) {
34
+ x = x * 2;
35
+ y = y + x;
36
+ }
37
+ $[0] = props;
38
+ $[1] = y;
39
} else {
30
- y = $[0];
31
- }
32
- for (
33
- let x = 0;
34
- x > props.min && x < props.max;
35
- x = x + (props.cond ? props.increment : 2), x
36
- ) {
37
- x = x * 2;
40
+ y = $[1];
41
}
42
return y;
43
}
compiler/forget/src/__tests__/fixtures/hir/ssa-cascading-eliminated-phis.expect.md
renamed
+2
-2
@@ -25,6 +25,7 @@ function Component(props) {
25
```javascript
26
function Component(props) {
27
const $ = React.unstable_useMemoCache(4);
28
+ let x = 0;
29
const c_0 = $[0] !== props;
30
let values;
31
if (c_0) {
@@ -40,12 +41,11 @@ function Component(props) {
41
}
42
const y = t0;
43
values.push(y);
43
- let x$0 = x;
44
if (props.c) {
45
x = 1;
46
}
47
48
- values.push(x$0);
48
+ values.push(x);
49
if (props.d) {
50
x = 2;
51
}