Skip to content

Commit 22638fa

Browse files
josephsavonajosephsavona
committed
[compiler] Add ImmutableCapture effect, CreateFrom no longer needs Capture
ImmutableCapture allows us to record information flow for escape analysis purposes, without impacting mutable range analysis. All of Alias, Capture, and CreateFrom now downgrade to ImmutableCapture if the `from` value is frozen. Globals and primitives are conceptually copy types and don't record any information flow at all. I guess we could add a `Copy` effect if we wanted but i haven't seen a need for it yet. Related, previously CreateFrom was always paired with Capture when constructing effects. But I had also coded up the inference to sort of treat them the same. So this diff makes that official, and means CreateFrom is a valid third way to represent data flow and not just creation. When applied, CreateFrom will turn into: ImmutableCapture for frozen values, Capture for mutable values, and disappear for globals/primitives. A final tweak is to change range inference to ensure that range.start is non-zero if the value is mutated. ghstack-source-id: fef144d Pull Request resolved: facebook/react#33367
1 parent 70caffd commit 22638fa

12 files changed

+308
-61
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -949,10 +949,13 @@ function getFunctionName(
949949
export function printAliasingEffect(effect: AliasingEffect): string {
950950
switch (effect.kind) {
951951
case 'Alias': {
952-
return `Alias ${printPlaceForAliasEffect(effect.from)} -> ${printPlaceForAliasEffect(effect.into)}`;
952+
return `Alias ${printPlaceForAliasEffect(effect.into)} = ${printPlaceForAliasEffect(effect.from)}`;
953953
}
954954
case 'Capture': {
955-
return `Capture ${printPlaceForAliasEffect(effect.from)} -> ${printPlaceForAliasEffect(effect.into)}`;
955+
return `Capture ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`;
956+
}
957+
case 'ImmutableCapture': {
958+
return `ImmutableCapture ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`;
956959
}
957960
case 'Create': {
958961
return `Create ${printPlaceForAliasEffect(effect.into)} = ${effect.value}`;

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
Place,
1515
isRefOrRefValue,
1616
makeInstructionId,
17+
printFunction,
1718
} from '../HIR';
1819
import {deadCodeElimination} from '../Optimization';
1920
import {inferReactiveScopeVariables} from '../ReactiveScopes';
@@ -26,10 +27,7 @@ import {
2627
eachInstructionValueOperand,
2728
} from '../HIR/visitors';
2829
import {Iterable_some} from '../Utils/utils';
29-
import {
30-
AliasingEffect,
31-
inferMutationAliasingEffects,
32-
} from './InferMutationAliasingEffects';
30+
import {inferMutationAliasingEffects} from './InferMutationAliasingEffects';
3331
import {inferMutationAliasingFunctionEffects} from './InferMutationAliasingFunctionEffects';
3432
import {inferMutationAliasingRanges} from './InferMutationAliasingRanges';
3533

@@ -44,7 +42,6 @@ export default function analyseFunctions(func: HIRFunction): void {
4442
infer(instr.value.loweredFunc, aliases);
4543
} else {
4644
lowerWithMutationAliasing(instr.value.loweredFunc.func);
47-
infer(instr.value.loweredFunc, new DisjointSet());
4845
}
4946

5047
/**

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

Lines changed: 88 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ import {
3838
Set_isSuperset,
3939
} from '../Utils/utils';
4040
import {
41+
printAliasingEffect,
4142
printIdentifier,
43+
printInstruction,
4244
printInstructionValue,
4345
printPlace,
4446
printSourceLocation,
@@ -258,6 +260,23 @@ function applySignature(
258260
state.define(effect.into, value);
259261
break;
260262
}
263+
case 'ImmutableCapture': {
264+
let value = effectInstructionValueCache.get(effect);
265+
if (value == null) {
266+
value = {
267+
kind: 'ObjectExpression',
268+
properties: [],
269+
loc: effect.into.loc,
270+
};
271+
effectInstructionValueCache.set(effect, value);
272+
}
273+
state.initialize(value, {
274+
kind: ValueKind.Frozen,
275+
reason: new Set([ValueReason.Other]),
276+
});
277+
state.define(effect.into, value);
278+
break;
279+
}
261280
case 'CreateFrom': {
262281
const kind = state.kind(effect.from).kind;
263282
let value = effectInstructionValueCache.get(effect);
@@ -274,6 +293,29 @@ function applySignature(
274293
reason: new Set([ValueReason.Other]),
275294
});
276295
state.define(effect.into, value);
296+
switch (kind) {
297+
case ValueKind.Primitive:
298+
case ValueKind.Global: {
299+
// no need to track this data flow
300+
break;
301+
}
302+
case ValueKind.Frozen: {
303+
effects.push({
304+
kind: 'ImmutableCapture',
305+
from: effect.from,
306+
into: effect.into,
307+
});
308+
break;
309+
}
310+
default: {
311+
// TODO: should this be Alias??
312+
effects.push({
313+
kind: 'Capture',
314+
from: effect.from,
315+
into: effect.into,
316+
});
317+
}
318+
}
277319
break;
278320
}
279321
case 'Capture': {
@@ -297,27 +339,28 @@ function applySignature(
297339
}
298340
}
299341
const fromKind = state.kind(effect.from).kind;
300-
let isCopyByReferenceValue: boolean;
342+
let isMutableReferenceType: boolean;
301343
switch (fromKind) {
302344
case ValueKind.Global:
303345
case ValueKind.Primitive: {
304-
isCopyByReferenceValue = false;
346+
isMutableReferenceType = false;
305347
break;
306348
}
307349
case ValueKind.Frozen: {
308-
/*
309-
* TODO: add a separate "ImmutableAlias" effect to downgrade to, that doesn't impact mutable ranges
310-
* We want to remember that the data flow occurred for PruneNonEscapingScopes
311-
*/
312-
isCopyByReferenceValue = false;
350+
isMutableReferenceType = false;
351+
effects.push({
352+
kind: 'ImmutableCapture',
353+
from: effect.from,
354+
into: effect.into,
355+
});
313356
break;
314357
}
315358
default: {
316-
isCopyByReferenceValue = true;
359+
isMutableReferenceType = true;
317360
break;
318361
}
319362
}
320-
if (isMutableDesination && isCopyByReferenceValue) {
363+
if (isMutableDesination && isMutableReferenceType) {
321364
effects.push(effect);
322365
}
323366
break;
@@ -329,12 +372,25 @@ function applySignature(
329372
*/
330373
const fromKind = state.kind(effect.from).kind;
331374
switch (fromKind) {
332-
/*
333-
* TODO: add a separate "ImmutableAlias" effect to downgrade to, that doesn't impact mutable ranges
334-
* We want to remember that the data flow occurred for PruneNonEscapingScopes
335-
* (use this to replace the ValueKind.Frozen case)
336-
*/
337-
case ValueKind.Frozen:
375+
case ValueKind.Frozen: {
376+
effects.push({
377+
kind: 'ImmutableCapture',
378+
from: effect.from,
379+
into: effect.into,
380+
});
381+
let value = effectInstructionValueCache.get(effect);
382+
if (value == null) {
383+
value = {
384+
kind: 'Primitive',
385+
value: undefined,
386+
loc: effect.from.loc,
387+
};
388+
effectInstructionValueCache.set(effect, value);
389+
}
390+
state.initialize(value, {kind: fromKind, reason: new Set([])});
391+
state.define(effect.into, value);
392+
break;
393+
}
338394
case ValueKind.Global:
339395
case ValueKind.Primitive: {
340396
let value = effectInstructionValueCache.get(effect);
@@ -385,24 +441,7 @@ function applySignature(
385441
case 'MutateTransitiveConditionally': {
386442
const didMutate = state.mutate(effect.kind, effect.value);
387443
if (didMutate) {
388-
switch (effect.kind) {
389-
case 'Mutate': {
390-
effects.push(effect);
391-
break;
392-
}
393-
case 'MutateConditionally': {
394-
effects.push({kind: 'Mutate', value: effect.value});
395-
break;
396-
}
397-
case 'MutateTransitive': {
398-
effects.push(effect);
399-
break;
400-
}
401-
case 'MutateTransitiveConditionally': {
402-
effects.push({kind: 'MutateTransitive', value: effect.value});
403-
break;
404-
}
405-
}
444+
effects.push(effect);
406445
}
407446
break;
408447
}
@@ -414,13 +453,19 @@ function applySignature(
414453
}
415454
}
416455
}
417-
CompilerError.invariant(
418-
state.isDefined(instruction.lvalue) && state.kind(instruction.lvalue),
419-
{
456+
if (
457+
!(state.isDefined(instruction.lvalue) && state.kind(instruction.lvalue))
458+
) {
459+
console.log(printInstruction(instruction));
460+
console.log('input effects');
461+
console.log(signature.effects.map(printAliasingEffect).join('\n'));
462+
console.log('refined effects');
463+
console.log(effects.map(printAliasingEffect).join('\n'));
464+
CompilerError.invariant(false, {
420465
reason: `Expected instruction lvalue to be initialized`,
421466
loc: instruction.loc,
422-
},
423-
);
467+
});
468+
}
424469
return effects.length !== 0 ? effects : null;
425470
}
426471

@@ -961,11 +1006,6 @@ function computeSignatureForInstruction(
9611006
from: value.object,
9621007
into: lvalue,
9631008
});
964-
effects.push({
965-
kind: 'Capture',
966-
from: value.object,
967-
into: lvalue,
968-
});
9691009
break;
9701010
}
9711011
case 'PropertyStore':
@@ -1106,11 +1146,6 @@ function computeSignatureForInstruction(
11061146
from: value.value,
11071147
into: patternLValue,
11081148
});
1109-
effects.push({
1110-
kind: 'Capture',
1111-
from: value.value,
1112-
into: patternLValue,
1113-
});
11141149
}
11151150
effects.push({kind: 'Alias', from: value.value, into: lvalue});
11161151
break;
@@ -1267,6 +1302,7 @@ function computeEffectsForSignature(
12671302
}
12681303
break;
12691304
}
1305+
case 'ImmutableCapture':
12701306
case 'CreateFrom':
12711307
case 'Apply':
12721308
case 'Mutate':
@@ -1413,6 +1449,10 @@ export type AliasingEffect =
14131449
* Creates a new value with the same kind as the starting value.
14141450
*/
14151451
| {kind: 'CreateFrom'; from: Place; into: Place}
1452+
/**
1453+
* Immutable data flow, used for escape analysis. Does not influence mutable range analysis:
1454+
*/
1455+
| {kind: 'ImmutableCapture'; from: Place; into: Place}
14161456
/**
14171457
* Calls the function at the given place with the given arguments either captured or aliased,
14181458
* and captures/aliases the result into the given place.

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

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
5454
effect.kind === 'Capture'
5555
) {
5656
captures.union([effect.from.identifier, effect.into.identifier]);
57-
} else if (effect.kind === 'MutateTransitive') {
57+
} else if (
58+
effect.kind === 'MutateTransitive' ||
59+
effect.kind === 'MutateTransitiveConditionally'
60+
) {
5861
const value = effect.value;
5962
value.identifier.mutableRange.end = makeInstructionId(instr.id + 1);
6063
}
@@ -83,7 +86,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
8386
for (const effect of instr.effects) {
8487
if (effect.kind === 'Alias' || effect.kind === 'CreateFrom') {
8588
aliases.union([effect.from.identifier, effect.into.identifier]);
86-
} else if (effect.kind === 'Mutate') {
89+
} else if (
90+
effect.kind === 'Mutate' ||
91+
effect.kind === 'MutateConditionally'
92+
) {
8793
const value = effect.value;
8894
value.identifier.mutableRange.end = makeInstructionId(instr.id + 1);
8995
}
@@ -94,7 +100,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
94100

95101
/**
96102
* Part 3
97-
* Add legacy operand-specific effects based on instruction effects and mutable ranges
103+
* Add legacy operand-specific effects based on instruction effects and mutable ranges.
104+
* Also fixes up operand mutable ranges, making sure that start is non-zero if the value
105+
* is mutated (depended on by later passes like InferReactiveScopeVariables which uses this
106+
* to filter spurious mutations of globals, which we now guard against more precisely)
98107
*/
99108
for (const block of fn.body.blocks.values()) {
100109
for (const phi of block.phis) {
@@ -136,6 +145,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
136145
}
137146
break;
138147
}
148+
case 'ImmutableCapture': {
149+
operandEffects.set(effect.from.identifier.id, Effect.Read);
150+
break;
151+
}
139152
case 'Create': {
140153
break;
141154
}
@@ -178,6 +191,12 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
178191
lvalue.effect = effect;
179192
}
180193
for (const operand of eachInstructionValueOperand(instr.value)) {
194+
if (
195+
operand.identifier.mutableRange.end > instr.id &&
196+
operand.identifier.mutableRange.start === 0
197+
) {
198+
operand.identifier.mutableRange.start = instr.id;
199+
}
181200
const effect = operandEffects.get(operand.identifier.id) ?? Effect.Read;
182201
operand.effect = effect;
183202
}

‎compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -376,12 +376,12 @@ export function findDisjointMutableValues(
376376
} else {
377377
for (const operand of eachInstructionOperand(instr)) {
378378
if (
379-
isMutable(instr, operand)
379+
isMutable(instr, operand) &&
380380
/*
381381
* exclude global variables from being added to scopes, we can't recreate them!
382382
* TODO: improve handling of module-scoped variables and globals
383383
*/
384-
// && operand.identifier.mutableRange.start > 0
384+
operand.identifier.mutableRange.start > 0
385385
) {
386386
if (
387387
instr.value.kind === 'FunctionExpression' ||

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ function Component(props) {
2222
7 | return hasErrors;
2323
8 | }
2424
> 9 | return hasErrors();
25-
| ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14 (9:9)
25+
| ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$15 (9:9)
2626
10 | }
2727
11 |
2828
```

0 commit comments

Comments
 (0)