Skip to content

Commit 4707109

Browse files
josephsavonajosephsavona
committed
[compiler] Infer phi types, extend mutable ranges to account for Store effects
Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: 8d8d282 Pull Request resolved: #30796
1 parent 798002c commit 4707109

File tree

6 files changed

+292
-29
lines changed

6 files changed

+292
-29
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,17 @@ export function inferMutableRanges(ir: HIRFunction): void {
5050
// Re-infer mutable ranges for all values
5151
inferMutableLifetimes(ir, true);
5252

53-
// Re-infer mutable ranges for aliases
54-
inferMutableRangesForAlias(ir, aliases);
53+
// Re-infer mutable ranges for aliases, but *not* for stores
54+
prevAliases = aliases.canonicalize();
55+
while (true) {
56+
inferMutableRangesForAlias(ir, aliases);
57+
inferAliasForPhis(ir, aliases);
58+
const nextAliases = aliases.canonicalize();
59+
if (areEqualMaps(prevAliases, nextAliases)) {
60+
break;
61+
}
62+
prevAliases = nextAliases;
63+
}
5564
}
5665

5766
function areEqualMaps<T>(a: Map<T, T>, b: Map<T, T>): boolean {

‎compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts‎

Lines changed: 112 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -483,30 +483,135 @@ class Unifier {
483483
}
484484

485485
if (type.kind === 'Phi') {
486-
const operands = new Set(type.operands.map(i => this.get(i).kind));
487-
488-
CompilerError.invariant(operands.size > 0, {
486+
CompilerError.invariant(type.operands.length > 0, {
489487
reason: 'there should be at least one operand',
490488
description: null,
491489
loc: null,
492490
suggestions: null,
493491
});
494-
const kind = operands.values().next().value;
495492

496-
// there's only one unique type and it's not a type var
497-
if (operands.size === 1 && kind !== 'Type') {
498-
this.unify(v, type.operands[0]);
493+
let candidateType: Type | null = null;
494+
for (const operand of type.operands) {
495+
const resolved = this.get(operand);
496+
if (candidateType === null) {
497+
candidateType = resolved;
498+
} else if (!typeEquals(resolved, candidateType)) {
499+
candidateType = null;
500+
break;
501+
} // else same type, continue
502+
}
503+
504+
if (candidateType !== null) {
505+
this.unify(v, candidateType);
499506
return;
500507
}
501508
}
502509

503510
if (this.occursCheck(v, type)) {
511+
const resolvedType = this.tryResolveType(v, type);
512+
if (resolvedType !== null) {
513+
this.substitutions.set(v.id, resolvedType);
514+
return;
515+
}
504516
throw new Error('cycle detected');
505517
}
506518

507519
this.substitutions.set(v.id, type);
508520
}
509521

522+
tryResolveType(v: TypeVar, type: Type): Type | null {
523+
switch (type.kind) {
524+
case 'Phi': {
525+
/**
526+
* Resolve the type of the phi by recursively removing `v` as an operand.
527+
* For example we can end up with types like this:
528+
*
529+
* v = Phi [
530+
* T1
531+
* T2
532+
* Phi [
533+
* T3
534+
* Phi [
535+
* T4
536+
* v <-- cycle!
537+
* ]
538+
* ]
539+
* ]
540+
*
541+
* By recursively removing `v`, we end up with:
542+
*
543+
* v = Phi [
544+
* T1
545+
* T2
546+
* Phi [
547+
* T3
548+
* Phi [
549+
* T4
550+
* ]
551+
* ]
552+
* ]
553+
*
554+
* Which avoids the cycle
555+
*/
556+
const operands = [];
557+
for (const operand of type.operands) {
558+
if (operand.kind === 'Type' && operand.id === v.id) {
559+
continue;
560+
}
561+
const resolved = this.tryResolveType(v, operand);
562+
if (resolved === null) {
563+
return null;
564+
}
565+
operands.push(resolved);
566+
}
567+
return {kind: 'Phi', operands};
568+
}
569+
case 'Type': {
570+
const substitution = this.get(type);
571+
if (substitution !== type) {
572+
const resolved = this.tryResolveType(v, substitution);
573+
if (resolved !== null) {
574+
this.substitutions.set(type.id, resolved);
575+
}
576+
return resolved;
577+
}
578+
return type;
579+
}
580+
case 'Property': {
581+
const objectType = this.tryResolveType(v, this.get(type.objectType));
582+
if (objectType === null) {
583+
return null;
584+
}
585+
return {
586+
kind: 'Property',
587+
objectName: type.objectName,
588+
objectType,
589+
propertyName: type.propertyName,
590+
};
591+
}
592+
case 'Function': {
593+
const returnType = this.tryResolveType(v, this.get(type.return));
594+
if (returnType === null) {
595+
return null;
596+
}
597+
return {
598+
kind: 'Function',
599+
return: returnType,
600+
shapeId: type.shapeId,
601+
};
602+
}
603+
case 'ObjectMethod':
604+
case 'Object':
605+
case 'Primitive':
606+
case 'Poly': {
607+
return type;
608+
}
609+
default: {
610+
assertExhaustive(type, `Unexpected type kind '${(type as any).kind}'`);
611+
}
612+
}
613+
}
614+
510615
occursCheck(v: TypeVar, type: Type): boolean {
511616
if (typeEquals(v, type)) return true;
512617

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/phi-type-inference-array-push-consecutive-phis.expect.md‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {makeArray} from 'shared-runtime';
6+
7+
function Component(props) {
8+
const x = {};
9+
let y;
10+
if (props.cond) {
11+
if (props.cond2) {
12+
y = [props.value];
13+
} else {
14+
y = [props.value2];
15+
}
16+
} else {
17+
y = [];
18+
}
19+
// This should be inferred as `<store> y` s.t. `x` can still
20+
// be independently memoized. *But* this also must properly
21+
// extend the mutable range of the array literals in the
22+
// if/else branches
23+
y.push(x);
24+
25+
return [x, y];
26+
}
27+
28+
export const FIXTURE_ENTRYPOINT = {
29+
fn: Component,
30+
params: [{cond: true, cond2: true, value: 42}],
31+
sequentialRenders: [
32+
{cond: true, cond2: true, value: 3.14},
33+
{cond: true, cond2: true, value: 42},
34+
{cond: true, cond2: true, value: 3.14},
35+
{cond: true, cond2: false, value2: 3.14},
36+
{cond: true, cond2: false, value2: 42},
37+
{cond: true, cond2: false, value2: 3.14},
38+
{cond: false},
39+
{cond: false},
40+
],
41+
};
42+
43+
```
44+
45+
## Code
46+
47+
```javascript
48+
import { c as _c } from "react/compiler-runtime";
49+
import { makeArray } from "shared-runtime";
50+
51+
function Component(props) {
52+
const $ = _c(3);
53+
let t0;
54+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
55+
t0 = {};
56+
$[0] = t0;
57+
} else {
58+
t0 = $[0];
59+
}
60+
const x = t0;
61+
let t1;
62+
if ($[1] !== props) {
63+
let y;
64+
if (props.cond) {
65+
if (props.cond2) {
66+
y = [props.value];
67+
} else {
68+
y = [props.value2];
69+
}
70+
} else {
71+
y = [];
72+
}
73+
74+
y.push(x);
75+
76+
t1 = [x, y];
77+
$[1] = props;
78+
$[2] = t1;
79+
} else {
80+
t1 = $[2];
81+
}
82+
return t1;
83+
}
84+
85+
export const FIXTURE_ENTRYPOINT = {
86+
fn: Component,
87+
params: [{ cond: true, cond2: true, value: 42 }],
88+
sequentialRenders: [
89+
{ cond: true, cond2: true, value: 3.14 },
90+
{ cond: true, cond2: true, value: 42 },
91+
{ cond: true, cond2: true, value: 3.14 },
92+
{ cond: true, cond2: false, value2: 3.14 },
93+
{ cond: true, cond2: false, value2: 42 },
94+
{ cond: true, cond2: false, value2: 3.14 },
95+
{ cond: false },
96+
{ cond: false },
97+
],
98+
};
99+
100+
```
101+
102+
### Eval output
103+
(kind: ok) [{},[3.14,"[[ cyclic ref *1 ]]"]]
104+
[{},[42,"[[ cyclic ref *1 ]]"]]
105+
[{},[3.14,"[[ cyclic ref *1 ]]"]]
106+
[{},[3.14,"[[ cyclic ref *1 ]]"]]
107+
[{},[42,"[[ cyclic ref *1 ]]"]]
108+
[{},[3.14,"[[ cyclic ref *1 ]]"]]
109+
[{},["[[ cyclic ref *1 ]]"]]
110+
[{},["[[ cyclic ref *1 ]]"]]

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/phi-type-inference-array-push-consecutive-phis.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import {makeArray} from 'shared-runtime';
2+
3+
function Component(props) {
4+
const x = {};
5+
let y;
6+
if (props.cond) {
7+
if (props.cond2) {
8+
y = [props.value];
9+
} else {
10+
y = [props.value2];
11+
}
12+
} else {
13+
y = [];
14+
}
15+
// This should be inferred as `<store> y` s.t. `x` can still
16+
// be independently memoized. *But* this also must properly
17+
// extend the mutable range of the array literals in the
18+
// if/else branches
19+
y.push(x);
20+
21+
return [x, y];
22+
}
23+
24+
export const FIXTURE_ENTRYPOINT = {
25+
fn: Component,
26+
params: [{cond: true, cond2: true, value: 42}],
27+
sequentialRenders: [
28+
{cond: true, cond2: true, value: 3.14},
29+
{cond: true, cond2: true, value: 42},
30+
{cond: true, cond2: true, value: 3.14},
31+
{cond: true, cond2: false, value2: 3.14},
32+
{cond: true, cond2: false, value2: 42},
33+
{cond: true, cond2: false, value2: 3.14},
34+
{cond: false},
35+
{cond: false},
36+
],
37+
};

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/phi-type-inference-array-push.expect.md‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,17 @@ export const FIXTURE_ENTRYPOINT = {
3636
```javascript
3737
import { c as _c } from "react/compiler-runtime";
3838
function Component(props) {
39-
const $ = _c(2);
39+
const $ = _c(3);
4040
let t0;
41-
if ($[0] !== props) {
42-
const x = {};
41+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
42+
t0 = {};
43+
$[0] = t0;
44+
} else {
45+
t0 = $[0];
46+
}
47+
const x = t0;
48+
let t1;
49+
if ($[1] !== props) {
4350
let y;
4451
if (props.cond) {
4552
y = [props.value];
@@ -49,13 +56,13 @@ function Component(props) {
4956

5057
y.push(x);
5158

52-
t0 = [x, y];
53-
$[0] = props;
54-
$[1] = t0;
59+
t1 = [x, y];
60+
$[1] = props;
61+
$[2] = t1;
5562
} else {
56-
t0 = $[1];
63+
t1 = $[2];
5764
}
58-
return t0;
65+
return t1;
5966
}
6067

6168
export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/phi-type-inference-property-store.expect.md‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export const FIXTURE_ENTRYPOINT = {
3232
```javascript
3333
import { c as _c } from "react/compiler-runtime"; // @debug
3434
function Component(props) {
35-
const $ = _c(5);
35+
const $ = _c(3);
3636
let t0;
3737
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
3838
t0 = {};
@@ -41,27 +41,22 @@ function Component(props) {
4141
t0 = $[0];
4242
}
4343
const x = t0;
44-
let y;
44+
let t1;
4545
if ($[1] !== props) {
46+
let y;
4647
if (props.cond) {
4748
y = {};
4849
} else {
4950
y = { a: props.a };
5051
}
5152

5253
y.x = x;
53-
$[1] = props;
54-
$[2] = y;
55-
} else {
56-
y = $[2];
57-
}
58-
let t1;
59-
if ($[3] !== y) {
54+
6055
t1 = [x, y];
61-
$[3] = y;
62-
$[4] = t1;
56+
$[1] = props;
57+
$[2] = t1;
6358
} else {
64-
t1 = $[4];
59+
t1 = $[2];
6560
}
6661
return t1;
6762
}

0 commit comments

Comments
 (0)