Skip to content

Commit d37faa0

Browse files
josephsavonajosephsavona
authored
[compiler] Preserve Create effects, guarantee effects initialize once (#33558)
Ensures that effects are well-formed with respect to the rules: * For a given instruction, each place is only initialized once (w one of Create, CreateFrom, Assign) * Ensures that Alias targets are already initialized within the same instruction (should have a Create before them) * Preserves Create and similar instructions * Avoids duplicate instructions when inferring effects of function expressions --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33558). * #33571 * __->__ #33558 * #33547
1 parent 3a2ff8b commit d37faa0

File tree

2 files changed

+88
-28
lines changed

2 files changed

+88
-28
lines changed

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,11 @@ import {inferReactiveScopeVariables} from '../ReactiveScopes';
2020
import {rewriteInstructionKindsBasedOnReassignment} from '../SSA';
2121
import {inferMutableRanges} from './InferMutableRanges';
2222
import inferReferenceEffects from './InferReferenceEffects';
23-
import {assertExhaustive} from '../Utils/utils';
23+
import {assertExhaustive, retainWhere} from '../Utils/utils';
2424
import {inferMutationAliasingEffects} from './InferMutationAliasingEffects';
2525
import {inferFunctionExpressionAliasingEffectsSignature} from './InferFunctionExpressionAliasingEffectsSignature';
2626
import {inferMutationAliasingRanges} from './InferMutationAliasingRanges';
27+
import {hashEffect} from './AliasingEffects';
2728

2829
export default function analyseFunctions(func: HIRFunction): void {
2930
for (const [_, block] of func.body.blocks) {
@@ -81,6 +82,17 @@ function lowerWithMutationAliasing(fn: HIRFunction): void {
8182
fn.aliasingEffects ??= [];
8283
fn.aliasingEffects?.push(...effects);
8384
}
85+
if (fn.aliasingEffects != null) {
86+
const seen = new Set<string>();
87+
retainWhere(fn.aliasingEffects, effect => {
88+
const hash = hashEffect(effect);
89+
if (seen.has(hash)) {
90+
return false;
91+
}
92+
seen.add(hash);
93+
return true;
94+
});
95+
}
8496

8597
/**
8698
* Phase 2: populate the Effect of each context variable to use in inferring

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

Lines changed: 75 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,11 @@ function inferBlock(
362362
} else if (terminal.kind === 'maybe-throw') {
363363
const handlerParam = context.catchHandlers.get(terminal.handler);
364364
if (handlerParam != null) {
365+
CompilerError.invariant(state.kind(handlerParam) != null, {
366+
reason:
367+
'Expected catch binding to be intialized with a DeclareLocal Catch instruction',
368+
loc: terminal.loc,
369+
});
365370
const effects: Array<AliasingEffect> = [];
366371
for (const instr of block.instructions) {
367372
if (
@@ -476,14 +481,14 @@ function applySignature(
476481
* Track which values we've already aliased once, so that we can switch to
477482
* appendAlias() for subsequent aliases into the same value
478483
*/
479-
const aliased = new Set<IdentifierId>();
484+
const initialized = new Set<IdentifierId>();
480485

481486
if (DEBUG) {
482487
console.log(printInstruction(instruction));
483488
}
484489

485490
for (const effect of signature.effects) {
486-
applyEffect(context, state, effect, aliased, effects);
491+
applyEffect(context, state, effect, initialized, effects);
487492
}
488493
if (DEBUG) {
489494
console.log(
@@ -508,7 +513,7 @@ function applyEffect(
508513
context: Context,
509514
state: InferenceState,
510515
_effect: AliasingEffect,
511-
aliased: Set<IdentifierId>,
516+
initialized: Set<IdentifierId>,
512517
effects: Array<AliasingEffect>,
513518
): void {
514519
const effect = context.internEffect(_effect);
@@ -524,6 +529,13 @@ function applyEffect(
524529
break;
525530
}
526531
case 'Create': {
532+
CompilerError.invariant(!initialized.has(effect.into.identifier.id), {
533+
reason: `Cannot re-initialize variable within an instruction`,
534+
description: `Re-initialized ${printPlace(effect.into)} in ${printAliasingEffect(effect)}`,
535+
loc: effect.into.loc,
536+
});
537+
initialized.add(effect.into.identifier.id);
538+
527539
let value = context.effectInstructionValueCache.get(effect);
528540
if (value == null) {
529541
value = {
@@ -538,6 +550,7 @@ function applyEffect(
538550
reason: new Set([effect.reason]),
539551
});
540552
state.define(effect.into, value);
553+
effects.push(effect);
541554
break;
542555
}
543556
case 'ImmutableCapture': {
@@ -555,6 +568,13 @@ function applyEffect(
555568
break;
556569
}
557570
case 'CreateFrom': {
571+
CompilerError.invariant(!initialized.has(effect.into.identifier.id), {
572+
reason: `Cannot re-initialize variable within an instruction`,
573+
description: `Re-initialized ${printPlace(effect.into)} in ${printAliasingEffect(effect)}`,
574+
loc: effect.into.loc,
575+
});
576+
initialized.add(effect.into.identifier.id);
577+
558578
const fromValue = state.kind(effect.from);
559579
let value = context.effectInstructionValueCache.get(effect);
560580
if (value == null) {
@@ -573,10 +593,21 @@ function applyEffect(
573593
switch (fromValue.kind) {
574594
case ValueKind.Primitive:
575595
case ValueKind.Global: {
576-
// no need to track this data flow
596+
effects.push({
597+
kind: 'Create',
598+
value: fromValue.kind,
599+
into: effect.into,
600+
reason: [...fromValue.reason][0] ?? ValueReason.Other,
601+
});
577602
break;
578603
}
579604
case ValueKind.Frozen: {
605+
effects.push({
606+
kind: 'Create',
607+
value: fromValue.kind,
608+
into: effect.into,
609+
reason: [...fromValue.reason][0] ?? ValueReason.Other,
610+
});
580611
applyEffect(
581612
context,
582613
state,
@@ -585,7 +616,7 @@ function applyEffect(
585616
from: effect.from,
586617
into: effect.into,
587618
},
588-
aliased,
619+
initialized,
589620
effects,
590621
);
591622
break;
@@ -597,6 +628,13 @@ function applyEffect(
597628
break;
598629
}
599630
case 'CreateFunction': {
631+
CompilerError.invariant(!initialized.has(effect.into.identifier.id), {
632+
reason: `Cannot re-initialize variable within an instruction`,
633+
description: `Re-initialized ${printPlace(effect.into)} in ${printAliasingEffect(effect)}`,
634+
loc: effect.into.loc,
635+
});
636+
initialized.add(effect.into.identifier.id);
637+
600638
effects.push(effect);
601639
/**
602640
* We consider the function mutable if it has any mutable context variables or
@@ -653,14 +691,22 @@ function applyEffect(
653691
from: capture,
654692
into: effect.into,
655693
},
656-
aliased,
694+
initialized,
657695
effects,
658696
);
659697
}
660698
break;
661699
}
662700
case 'Alias':
663701
case 'Capture': {
702+
CompilerError.invariant(
703+
effect.kind === 'Capture' || initialized.has(effect.into.identifier.id),
704+
{
705+
reason: `Expected destination value to already be initialized within this instruction for Alias effect`,
706+
description: `Destination ${printPlace(effect.into)} is not initialized in this instruction`,
707+
loc: effect.into.loc,
708+
},
709+
);
664710
/*
665711
* Capture describes potential information flow: storing a pointer to one value
666712
* within another. If the destination is not mutable, or the source value has
@@ -698,7 +744,7 @@ function applyEffect(
698744
from: effect.from,
699745
into: effect.into,
700746
},
701-
aliased,
747+
initialized,
702748
effects,
703749
);
704750
break;
@@ -714,6 +760,13 @@ function applyEffect(
714760
break;
715761
}
716762
case 'Assign': {
763+
CompilerError.invariant(!initialized.has(effect.into.identifier.id), {
764+
reason: `Cannot re-initialize variable within an instruction`,
765+
description: `Re-initialized ${printPlace(effect.into)} in ${printAliasingEffect(effect)}`,
766+
loc: effect.into.loc,
767+
});
768+
initialized.add(effect.into.identifier.id);
769+
717770
/*
718771
* Alias represents potential pointer aliasing. If the type is a global,
719772
* a primitive (copy-on-write semantics) then we can prune the effect
@@ -730,7 +783,7 @@ function applyEffect(
730783
from: effect.from,
731784
into: effect.into,
732785
},
733-
aliased,
786+
initialized,
734787
effects,
735788
);
736789
let value = context.effectInstructionValueCache.get(effect);
@@ -768,12 +821,7 @@ function applyEffect(
768821
break;
769822
}
770823
default: {
771-
if (aliased.has(effect.into.identifier.id)) {
772-
state.appendAlias(effect.into, effect.from);
773-
} else {
774-
aliased.add(effect.into.identifier.id);
775-
state.alias(effect.into, effect.from);
776-
}
824+
state.assign(effect.into, effect.from);
777825
effects.push(effect);
778826
break;
779827
}
@@ -826,11 +874,11 @@ function applyEffect(
826874
context,
827875
state,
828876
{kind: 'MutateTransitiveConditionally', value: effect.function},
829-
aliased,
877+
initialized,
830878
effects,
831879
);
832880
for (const signatureEffect of signatureEffects) {
833-
applyEffect(context, state, signatureEffect, aliased, effects);
881+
applyEffect(context, state, signatureEffect, initialized, effects);
834882
}
835883
break;
836884
}
@@ -858,7 +906,7 @@ function applyEffect(
858906
console.log('apply aliasing signature effects');
859907
}
860908
for (const signatureEffect of signatureEffects) {
861-
applyEffect(context, state, signatureEffect, aliased, effects);
909+
applyEffect(context, state, signatureEffect, initialized, effects);
862910
}
863911
} else if (effect.signature != null) {
864912
if (DEBUG) {
@@ -873,7 +921,7 @@ function applyEffect(
873921
effect.loc,
874922
);
875923
for (const legacyEffect of legacyEffects) {
876-
applyEffect(context, state, legacyEffect, aliased, effects);
924+
applyEffect(context, state, legacyEffect, initialized, effects);
877925
}
878926
} else {
879927
if (DEBUG) {
@@ -888,7 +936,7 @@ function applyEffect(
888936
value: ValueKind.Mutable,
889937
reason: ValueReason.Other,
890938
},
891-
aliased,
939+
initialized,
892940
effects,
893941
);
894942
/*
@@ -911,21 +959,21 @@ function applyEffect(
911959
kind: 'MutateTransitiveConditionally',
912960
value: operand,
913961
},
914-
aliased,
962+
initialized,
915963
effects,
916964
);
917965
}
918966
const mutateIterator =
919967
arg.kind === 'Spread' ? conditionallyMutateIterator(operand) : null;
920968
if (mutateIterator) {
921-
applyEffect(context, state, mutateIterator, aliased, effects);
969+
applyEffect(context, state, mutateIterator, initialized, effects);
922970
}
923971
applyEffect(
924972
context,
925973
state,
926974
// OK: recording information flow
927975
{kind: 'Alias', from: operand, into: effect.into},
928-
aliased,
976+
initialized,
929977
effects,
930978
);
931979
for (const otherArg of [
@@ -953,7 +1001,7 @@ function applyEffect(
9531001
from: operand,
9541002
into: other,
9551003
},
956-
aliased,
1004+
initialized,
9571005
effects,
9581006
);
9591007
}
@@ -1009,7 +1057,7 @@ function applyEffect(
10091057
suggestions: null,
10101058
},
10111059
},
1012-
aliased,
1060+
initialized,
10131061
effects,
10141062
);
10151063
}
@@ -1028,7 +1076,7 @@ function applyEffect(
10281076
suggestions: null,
10291077
},
10301078
},
1031-
aliased,
1079+
initialized,
10321080
effects,
10331081
);
10341082
} else {
@@ -1059,7 +1107,7 @@ function applyEffect(
10591107
suggestions: null,
10601108
},
10611109
},
1062-
aliased,
1110+
initialized,
10631111
effects,
10641112
);
10651113
}
@@ -1166,7 +1214,7 @@ class InferenceState {
11661214
}
11671215

11681216
// Updates the value at @param place to point to the same value as @param value.
1169-
alias(place: Place, value: Place): void {
1217+
assign(place: Place, value: Place): void {
11701218
const values = this.#variables.get(value.identifier.id);
11711219
CompilerError.invariant(values != null, {
11721220
reason: `[InferMutationAliasingEffects] Expected value for identifier to be initialized`,

0 commit comments

Comments
 (0)