Skip to content

Commit

Permalink
Update
Browse files Browse the repository at this point in the history 
[ghstack-poisoned]
  • Loading branch information
@mofeiZ
mofeiZ committed Sep 30, 2024
1 parent 23561fc commit 9119289
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 48 deletions.
53 changes: 34 additions & 19 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import {
} from './HIR';

/**
* Helper function for `PropagateScopeDependencies`.
* Uses control flow graph analysis to determine which `Identifier`s can
* be assumed to be non-null objects, on a per-block basis.
* Helper function for `PropagateScopeDependencies`. Uses control flow graph
* analysis to determine which `Identifier`s can be assumed to be non-null
* objects, on a per-block basis.
*
* Here is an example:
* ```js
Expand All @@ -48,15 +48,16 @@ import {
* }
* ```
*
* Note that we currently do NOT account for mutable / declaration range
* when doing the CFG-based traversal, producing results that are technically
* Note that we currently do NOT account for mutable / declaration range when
* doing the CFG-based traversal, producing results that are technically
* incorrect but filtered by PropagateScopeDeps (which only takes dependencies
* on constructed value -- i.e. a scope's dependencies must have mutable ranges
* ending earlier than the scope start).
*
* Take this example, this function will infer x.foo.bar as non-nullable for bb0,
* via the intersection of bb1 & bb2 which in turn comes from bb3. This is technically
* incorrect bb0 is before / during x's mutable range.
* Take this example, this function will infer x.foo.bar as non-nullable for
* bb0, via the intersection of bb1 & bb2 which in turn comes from bb3. This is
* technically incorrect bb0 is before / during x's mutable range.
* ```
* bb0:
* const x = ...;
* if cond then bb1 else bb2
Expand All @@ -68,15 +69,29 @@ import {
* goto bb3:
* bb3:
* x.foo.bar
* ```
*
* @param fn
* @param temporaries sidemap of identifier -> baseObject.a.b paths. Does not
* contain optional chains.
* @param hoistableFromOptionals sidemap of optionalBlock -> baseObject?.a
* optional paths for which it's safe to evaluate non-optional loads (see
* CollectOptionalChainDependencies).
* @returns
*/
export function collectHoistablePropertyLoads(
fn: HIRFunction,
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
optionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
): ReadonlyMap<ScopeId, BlockInfo> {
const registry = new PropertyPathRegistry();

const nodes = collectNonNullsInBlocks(fn, temporaries, optionals, registry);
const nodes = collectNonNullsInBlocks(
fn,
temporaries,
hoistableFromOptionals,
registry,
);
propagateNonNull(fn, nodes, registry);

const nodesKeyedByScopeId = new Map<ScopeId, BlockInfo>();
Expand Down Expand Up @@ -226,7 +241,7 @@ function addNonNullPropertyPath(
function collectNonNullsInBlocks(
fn: HIRFunction,
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
optionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
registry: PropertyPathRegistry,
): ReadonlyMap<BlockId, BlockInfo> {
/**
Expand Down Expand Up @@ -268,7 +283,7 @@ function collectNonNullsInBlocks(
block,
assumedNonNullObjects,
});
const maybeOptionalChain = optionals.get(block.id);
const maybeOptionalChain = hoistableFromOptionals.get(block.id);
if (maybeOptionalChain != null) {
assumedNonNullObjects.add(
registry.getOrCreateProperty(maybeOptionalChain),
Expand Down Expand Up @@ -412,8 +427,10 @@ function propagateNonNull(
assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects;
traversalState.set(nodeId, 'done');
/**
* Note that it might not sufficient to compare set sizes since reduceMaybeOptionalChains
* may replace optional-chain loads with unconditional loads
* Note that it's not sufficient to compare set sizes since
* reduceMaybeOptionalChains may replace optional-chain loads with
* unconditional loads. This could in turn change `assumedNonNullObjects` of
* downstream blocks and backedges.
*/
changed ||= !Set_equal(prevObjects, mergedObjects);
return changed;
Expand Down Expand Up @@ -470,8 +487,8 @@ export function assertNonNull<T extends NonNullable<U>, U>(
* property strings paths de-duplicates.
*
* Intuitively: given <base>?.b, we know <base> to be either hoistable or not.
* If <base> is hoistable, we can replace all <base>?.PROPERTY_STRING subpaths
* with <base>.PROPERTY_STRING
* If unconditional reads from <base> are hoistable, we can replace all
* <base>?.PROPERTY_STRING subpaths with <base>.PROPERTY_STRING
*/
function reduceMaybeOptionalChains(
nodes: Set<PropertyPathNode>,
Expand All @@ -481,7 +498,6 @@ function reduceMaybeOptionalChains(
if (optionalChainNodes.size === 0) {
return;
}
const knownNonNulls = new Set(nodes);
let changed: boolean;
do {
changed = false;
Expand All @@ -494,7 +510,7 @@ function reduceMaybeOptionalChains(
const entry = origPath[i];
// If the base is known to be non-null, replace with a non-optional load
const nextEntry: DependencyPathEntry =
entry.optional && knownNonNulls.has(currNode)
entry.optional && nodes.has(currNode)
? {property: entry.property, optional: false}
: entry;
currNode = PropertyPathRegistry.getOrCreatePropertyEntry(
Expand All @@ -508,7 +524,6 @@ function reduceMaybeOptionalChains(
optionalChainNodes.add(currNode);
nodes.delete(original);
nodes.add(currNode);
knownNonNulls.add(currNode);
}
}
} while (changed);
Expand Down
59 changes: 30 additions & 29 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import {
OptionalTerminal,
HIRFunction,
} from './HIR';
import {printIdentifier, printInstruction} from './PrintHIR';
import {printIdentifier} from './PrintHIR';

export function collectOptionalChainSidemap(
fn: HIRFunction,
): OptionalChainSidemap {
Expand Down Expand Up @@ -49,12 +50,14 @@ export function collectOptionalChainSidemap(
export type OptionalChainSidemap = {
/**
* Stores the correct property mapping (e.g. `a?.b` instead of `a.b`) for
* dependency calculation. Note that we currently do not store anything on phi nodes (e.g. the outer )
* dependency calculation. Note that we currently do not store anything on
* outer phi nodes.
*/
temporariesReadInOptional: ReadonlyMap<IdentifierId, ReactiveScopeDependency>;
/**
* When extracting dependencies in PropagateScopeDependencies, skip instructions already
* processed in this pass.
* Records instructions (PropertyLoads, StoreLocals, and test terminals)
* processed in this pass. When extracting dependencies in
* PropagateScopeDependencies, these instructions are skipped.
*
* E.g. given a?.b
* ```
Expand Down Expand Up @@ -96,9 +99,9 @@ export type OptionalChainSidemap = {
*/
processedInstrsInOptional: ReadonlySet<InstructionId>;
/**
* Optional-chains that can be hoisted to the start of optional chains. e.g.
* given `a?.b.c`, we can evaluate any PropertyLoad from `a?.b` at the
* optional terminal in bb1
* Records optional chains for which we can safely evaluate non-optional
* PropertyLoads. e.g. given `a?.b.c`, we can evaluate any load from `a?.b` at
* the optional terminal in bb1.
* ```js
* bb1:
* ...
Expand Down Expand Up @@ -196,11 +199,12 @@ function assertOptionalAlternateBlock(

/**
* Traverse into the optional block and all transitively referenced blocks to
* collect a sidemaps identifier and block ids -> optional chain dependencies.
* collect sidemaps of optional chain dependencies.
*
* @returns the IdentifierId representing the optional block if the block and
* all transitively referenced optionals precisely represent a chain of property
* loads. If any part of the optional chain is not hoistable, returns null.
* all transitively referenced optional blocks precisely represent a chain of
* property loads. If any part of the optional chain is not hoistable, returns
* null.
*/
function traverseOptionalBlock(
optional: TBasicBlock<OptionalTerminal>,
Expand All @@ -224,20 +228,16 @@ function traverseOptionalBlock(
* blocks, but this would require changes to HIR.
*/
CompilerError.invariant(optional.terminal.optional, {
reason:
'[OptionalChainDeps] Expect base optional case to be always optional',
reason: '[OptionalChainDeps] Expect base case to be always optional',
loc: optional.terminal.loc,
});
CompilerError.invariant(maybeTest.instructions.length >= 1, {
reason:
'[OptionalChainDeps] Expected direct optional test branch (base case) to have at least one instruction',
loc: maybeTest.terminal.loc,
});

/**
* Only match base expressions that are straightforward PropertyLoad chains
*/
if (maybeTest.instructions[0].value.kind !== 'LoadLocal') {
if (
maybeTest.instructions.length === 0 ||
maybeTest.instructions[0].value.kind !== 'LoadLocal'
) {
return null;
}
const path = maybeTest.instructions.slice(1).map((entry, i) => {
Expand Down Expand Up @@ -271,7 +271,9 @@ function traverseOptionalBlock(
} else if (maybeTest.terminal.kind === 'optional') {
/**
* This is either
* - a chained optional i.e. base=<inner_optional>?.b or <inner_optional>.b
* - <inner_optional>?.property (optional=true)
* - <inner_optional>.property (optional=false)
* - <inner_optional> <other operation>
* - a optional base block with a separate nested optional-chain (e.g. a(c?.d)?.d)
*/
const testBlock = context.blocks.get(maybeTest.terminal.fallthrough)!;
Expand Down Expand Up @@ -337,19 +339,18 @@ function traverseOptionalBlock(
context.temporariesReadInOptional.get(innerOptional),
);
test = testBlock.terminal;
if (test.alternate === outerAlternate) {
CompilerError.invariant(optional.instructions.length === 0, {
reason:
'[OptionalChainDeps] Unexpected instructions an inner optional block. ' +
'This indicates that the compiler may be incorrectly concatenating two unrelated optional chains',
description: `bb${optional.id}\n ${testBlock.id}\n${optional.instructions.map(printInstruction).join('\n')}`,
loc: optional.terminal.loc,
});
}
} else {
return null;
}

if (test.alternate === outerAlternate) {
CompilerError.invariant(optional.instructions.length === 0, {
reason:
'[OptionalChainDeps] Unexpected instructions an inner optional block. ' +
'This indicates that the compiler may be incorrectly concatenating two unrelated optional chains',
loc: optional.terminal.loc,
});
}
const matchConsequentResult = matchOptionalTestBlock(test, context.blocks);
if (!matchConsequentResult) {
// Optional chain consequent is not hoistable e.g. a?.[computed()]
Expand Down

0 comments on commit 9119289

Please sign in to comment.