Skip to content

Commit c67e241

Browse files
committed
[compiler] Renames and no-op refactor for next PR
Rename for clarity: - `CollectHoistablePropertyLoads:Tree` -> `CollectHoistablePropertyLoads:PropertyPathRegistry` - `getPropertyLoadNode` -> `getOrCreateProperty` - `getOrCreateRoot` -> `getOrCreateIdentifier` - `PropertyLoadNode` -> `PropertyPathNode` Refactor to CFG joining logic for `CollectHoistablePropertyLoads`. We now write to the same set of inferredNonNullObjects when traversing from entry and exit blocks. This is more correct, as non-nulls inferred from a forward traversal should be included when computing the backward traversal (and vice versa). This fix is needed by an edge case in #31036 Added invariant into fixed-point iteration to terminate (instead of infinite looping). ghstack-source-id: 1e8eb2d Pull Request resolved: #31036
1 parent 2cbea24 commit c67e241

File tree

3 files changed

+95
-108
lines changed

3 files changed

+95
-108
lines changed

compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts

Lines changed: 69 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {Set_intersect, Set_union, getOrInsertDefault} from '../Utils/utils';
44
import {
55
BasicBlock,
66
BlockId,
7+
DependencyPathEntry,
78
GeneratedSource,
89
HIRFunction,
910
Identifier,
@@ -66,7 +67,9 @@ export function collectHoistablePropertyLoads(
6667
fn: HIRFunction,
6768
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
6869
): ReadonlyMap<ScopeId, BlockInfo> {
69-
const nodes = collectNonNullsInBlocks(fn, temporaries);
70+
const registry = new PropertyPathRegistry();
71+
72+
const nodes = collectNonNullsInBlocks(fn, temporaries, registry);
7073
propagateNonNull(fn, nodes);
7174

7275
const nodesKeyedByScopeId = new Map<ScopeId, BlockInfo>();
@@ -84,33 +87,33 @@ export function collectHoistablePropertyLoads(
8487

8588
export type BlockInfo = {
8689
block: BasicBlock;
87-
assumedNonNullObjects: ReadonlySet<PropertyLoadNode>;
90+
assumedNonNullObjects: ReadonlySet<PropertyPathNode>;
8891
};
8992

9093
/**
91-
* Tree data structure to dedupe property loads (e.g. a.b.c)
94+
* PropertyLoadRegistry data structure to dedupe property loads (e.g. a.b.c)
9295
* and make computing sets intersections simpler.
9396
*/
9497
type RootNode = {
95-
properties: Map<string, PropertyLoadNode>;
98+
properties: Map<string, PropertyPathNode>;
9699
parent: null;
97100
// Recorded to make later computations simpler
98101
fullPath: ReactiveScopeDependency;
99102
root: IdentifierId;
100103
};
101104

102-
type PropertyLoadNode =
105+
type PropertyPathNode =
103106
| {
104-
properties: Map<string, PropertyLoadNode>;
105-
parent: PropertyLoadNode;
107+
properties: Map<string, PropertyPathNode>;
108+
parent: PropertyPathNode;
106109
fullPath: ReactiveScopeDependency;
107110
}
108111
| RootNode;
109112

110-
class Tree {
113+
class PropertyPathRegistry {
111114
roots: Map<IdentifierId, RootNode> = new Map();
112115

113-
getOrCreateRoot(identifier: Identifier): PropertyLoadNode {
116+
getOrCreateIdentifier(identifier: Identifier): PropertyPathNode {
114117
/**
115118
* Reads from a statically scoped variable are always safe in JS,
116119
* with the exception of TDZ (not addressed by this pass).
@@ -132,49 +135,61 @@ class Tree {
132135
return rootNode;
133136
}
134137

135-
static #getOrCreateProperty(
136-
node: PropertyLoadNode,
137-
property: string,
138-
): PropertyLoadNode {
139-
let child = node.properties.get(property);
138+
static getOrCreatePropertyEntry(
139+
parent: PropertyPathNode,
140+
entry: DependencyPathEntry,
141+
): PropertyPathNode {
142+
if (entry.optional) {
143+
CompilerError.throwTodo({
144+
reason: 'handle optional nodes',
145+
loc: GeneratedSource,
146+
});
147+
}
148+
let child = parent.properties.get(entry.property);
140149
if (child == null) {
141150
child = {
142151
properties: new Map(),
143-
parent: node,
152+
parent: parent,
144153
fullPath: {
145-
identifier: node.fullPath.identifier,
146-
path: node.fullPath.path.concat([{property, optional: false}]),
154+
identifier: parent.fullPath.identifier,
155+
path: parent.fullPath.path.concat(entry),
147156
},
148157
};
149-
node.properties.set(property, child);
158+
parent.properties.set(entry.property, child);
150159
}
151160
return child;
152161
}
153162

154-
getPropertyLoadNode(n: ReactiveScopeDependency): PropertyLoadNode {
163+
getOrCreateProperty(n: ReactiveScopeDependency): PropertyPathNode {
155164
/**
156165
* We add ReactiveScopeDependencies according to instruction ordering,
157166
* so all subpaths of a PropertyLoad should already exist
158167
* (e.g. a.b is added before a.b.c),
159168
*/
160-
let currNode = this.getOrCreateRoot(n.identifier);
169+
let currNode = this.getOrCreateIdentifier(n.identifier);
161170
if (n.path.length === 0) {
162171
return currNode;
163172
}
164173
for (let i = 0; i < n.path.length - 1; i++) {
165-
currNode = assertNonNull(currNode.properties.get(n.path[i].property));
174+
currNode = PropertyPathRegistry.getOrCreatePropertyEntry(
175+
currNode,
176+
n.path[i],
177+
);
166178
}
167179

168-
return Tree.#getOrCreateProperty(currNode, n.path.at(-1)!.property);
180+
return PropertyPathRegistry.getOrCreatePropertyEntry(
181+
currNode,
182+
n.path.at(-1)!,
183+
);
169184
}
170185
}
171186

172-
function pushPropertyLoadNode(
173-
loadSource: Identifier,
174-
loadSourceNode: PropertyLoadNode,
187+
function addNonNullPropertyPath(
188+
source: Identifier,
189+
sourceNode: PropertyPathNode,
175190
instrId: InstructionId,
176191
knownImmutableIdentifiers: Set<IdentifierId>,
177-
result: Set<PropertyLoadNode>,
192+
result: Set<PropertyPathNode>,
178193
): void {
179194
/**
180195
* Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges
@@ -187,26 +202,22 @@ function pushPropertyLoadNode(
187202
* See comment at top of function for why we track known immutable identifiers.
188203
*/
189204
const isMutableAtInstr =
190-
loadSource.mutableRange.end > loadSource.mutableRange.start + 1 &&
191-
loadSource.scope != null &&
192-
inRange({id: instrId}, loadSource.scope.range);
205+
source.mutableRange.end > source.mutableRange.start + 1 &&
206+
source.scope != null &&
207+
inRange({id: instrId}, source.scope.range);
193208
if (
194209
!isMutableAtInstr ||
195-
knownImmutableIdentifiers.has(loadSourceNode.fullPath.identifier.id)
210+
knownImmutableIdentifiers.has(sourceNode.fullPath.identifier.id)
196211
) {
197-
let curr: PropertyLoadNode | null = loadSourceNode;
198-
while (curr != null) {
199-
result.add(curr);
200-
curr = curr.parent;
201-
}
212+
result.add(sourceNode);
202213
}
203214
}
204215

205216
function collectNonNullsInBlocks(
206217
fn: HIRFunction,
207218
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
219+
registry: PropertyPathRegistry,
208220
): ReadonlyMap<BlockId, BlockInfo> {
209-
const tree = new Tree();
210221
/**
211222
* Due to current limitations of mutable range inference, there are edge cases in
212223
* which we infer known-immutable values (e.g. props or hook params) to have a
@@ -227,18 +238,18 @@ function collectNonNullsInBlocks(
227238
* Known non-null objects such as functional component props can be safely
228239
* read from any block.
229240
*/
230-
const knownNonNullIdentifiers = new Set<PropertyLoadNode>();
241+
const knownNonNullIdentifiers = new Set<PropertyPathNode>();
231242
if (
232243
fn.fnType === 'Component' &&
233244
fn.params.length > 0 &&
234245
fn.params[0].kind === 'Identifier'
235246
) {
236247
const identifier = fn.params[0].identifier;
237-
knownNonNullIdentifiers.add(tree.getOrCreateRoot(identifier));
248+
knownNonNullIdentifiers.add(registry.getOrCreateIdentifier(identifier));
238249
}
239250
const nodes = new Map<BlockId, BlockInfo>();
240251
for (const [_, block] of fn.body.blocks) {
241-
const assumedNonNullObjects = new Set<PropertyLoadNode>(
252+
const assumedNonNullObjects = new Set<PropertyPathNode>(
242253
knownNonNullIdentifiers,
243254
);
244255
for (const instr of block.instructions) {
@@ -247,9 +258,9 @@ function collectNonNullsInBlocks(
247258
identifier: instr.value.object.identifier,
248259
path: [],
249260
};
250-
pushPropertyLoadNode(
261+
addNonNullPropertyPath(
251262
instr.value.object.identifier,
252-
tree.getPropertyLoadNode(source),
263+
registry.getOrCreateProperty(source),
253264
instr.id,
254265
knownImmutableIdentifiers,
255266
assumedNonNullObjects,
@@ -258,9 +269,9 @@ function collectNonNullsInBlocks(
258269
const source = instr.value.value.identifier.id;
259270
const sourceNode = temporaries.get(source);
260271
if (sourceNode != null) {
261-
pushPropertyLoadNode(
272+
addNonNullPropertyPath(
262273
instr.value.value.identifier,
263-
tree.getPropertyLoadNode(sourceNode),
274+
registry.getOrCreateProperty(sourceNode),
264275
instr.id,
265276
knownImmutableIdentifiers,
266277
assumedNonNullObjects,
@@ -270,9 +281,9 @@ function collectNonNullsInBlocks(
270281
const source = instr.value.object.identifier.id;
271282
const sourceNode = temporaries.get(source);
272283
if (sourceNode != null) {
273-
pushPropertyLoadNode(
284+
addNonNullPropertyPath(
274285
instr.value.object.identifier,
275-
tree.getPropertyLoadNode(sourceNode),
286+
registry.getOrCreateProperty(sourceNode),
276287
instr.id,
277288
knownImmutableIdentifiers,
278289
assumedNonNullObjects,
@@ -314,7 +325,6 @@ function propagateNonNull(
314325
nodeId: BlockId,
315326
direction: 'forward' | 'backward',
316327
traversalState: Map<BlockId, 'active' | 'done'>,
317-
nonNullObjectsByBlock: Map<BlockId, ReadonlySet<PropertyLoadNode>>,
318328
): boolean {
319329
/**
320330
* Avoid re-visiting computed or currently active nodes, which can
@@ -345,7 +355,6 @@ function propagateNonNull(
345355
pred,
346356
direction,
347357
traversalState,
348-
nonNullObjectsByBlock,
349358
);
350359
changed ||= neighborChanged;
351360
}
@@ -374,38 +383,36 @@ function propagateNonNull(
374383
const neighborAccesses = Set_intersect(
375384
Array.from(neighbors)
376385
.filter(n => traversalState.get(n) === 'done')
377-
.map(n => assertNonNull(nonNullObjectsByBlock.get(n))),
386+
.map(n => assertNonNull(nodes.get(n)).assumedNonNullObjects),
378387
);
379388

380-
const prevObjects = assertNonNull(nonNullObjectsByBlock.get(nodeId));
381-
const newObjects = Set_union(prevObjects, neighborAccesses);
389+
const prevObjects = assertNonNull(nodes.get(nodeId)).assumedNonNullObjects;
390+
const mergedObjects = Set_union(prevObjects, neighborAccesses);
382391

383-
nonNullObjectsByBlock.set(nodeId, newObjects);
392+
assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects;
384393
traversalState.set(nodeId, 'done');
385-
changed ||= prevObjects.size !== newObjects.size;
394+
changed ||= prevObjects.size !== mergedObjects.size;
386395
return changed;
387396
}
388-
const fromEntry = new Map<BlockId, ReadonlySet<PropertyLoadNode>>();
389-
const fromExit = new Map<BlockId, ReadonlySet<PropertyLoadNode>>();
390-
for (const [blockId, blockInfo] of nodes) {
391-
fromEntry.set(blockId, blockInfo.assumedNonNullObjects);
392-
fromExit.set(blockId, blockInfo.assumedNonNullObjects);
393-
}
394397
const traversalState = new Map<BlockId, 'done' | 'active'>();
395398
const reversedBlocks = [...fn.body.blocks];
396399
reversedBlocks.reverse();
397400

398-
let i = 0;
399401
let changed;
402+
let i = 0;
400403
do {
401-
i++;
404+
CompilerError.invariant(i++ < 100, {
405+
reason:
406+
'[CollectHoistablePropertyLoads] fixed point iteration did not terminate after 100 loops',
407+
loc: GeneratedSource,
408+
});
409+
402410
changed = false;
403411
for (const [blockId] of fn.body.blocks) {
404412
const forwardChanged = recursivelyPropagateNonNull(
405413
blockId,
406414
'forward',
407415
traversalState,
408-
fromEntry,
409416
);
410417
changed ||= forwardChanged;
411418
}
@@ -415,43 +422,14 @@ function propagateNonNull(
415422
blockId,
416423
'backward',
417424
traversalState,
418-
fromExit,
419425
);
420426
changed ||= backwardChanged;
421427
}
422428
traversalState.clear();
423429
} while (changed);
424-
425-
/**
426-
* TODO: validate against meta internal code, then remove in future PR.
427-
* Currently cannot come up with a case that requires fixed-point iteration.
428-
*/
429-
CompilerError.invariant(i <= 2, {
430-
reason: 'require fixed-point iteration',
431-
description: `#iterations = ${i}`,
432-
loc: GeneratedSource,
433-
});
434-
435-
CompilerError.invariant(
436-
fromEntry.size === fromExit.size && fromEntry.size === nodes.size,
437-
{
438-
reason:
439-
'bad sizes after calculating fromEntry + fromExit ' +
440-
`${fromEntry.size} ${fromExit.size} ${nodes.size}`,
441-
loc: GeneratedSource,
442-
},
443-
);
444-
445-
for (const [id, node] of nodes) {
446-
const assumedNonNullObjects = Set_union(
447-
assertNonNull(fromEntry.get(id)),
448-
assertNonNull(fromExit.get(id)),
449-
);
450-
node.assumedNonNullObjects = assumedNonNullObjects;
451-
}
452430
}
453431

454-
function assertNonNull<T extends NonNullable<U>, U>(
432+
export function assertNonNull<T extends NonNullable<U>, U>(
455433
value: T | null | undefined,
456434
source?: string,
457435
): T {

0 commit comments

Comments
 (0)