[compiler] Migrate PruneNonEscapingScopes to HIR #31882
Conversation
Summary: PruneNonEscapingScopes does a pretty powerful escape analysis, which we might want to apply for other purposes in our HIR passes. This ports this pass to HIR. For the most part, this implementation is identical to the ReactiveFunction version. It now handles phis instead of conditional ReactiveExpressions, which it does by treating all the phi operands as possibly aliasing the lvalue. This also requires that we iterate the aliasing analysis to a fixpoint, because the HIR has backedges which the ReactiveFunctions don't. In our fixtures, this only changes one result, which appears to have become more accurate. I plan on testing this internally in a sync before landing. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary: PruneNonEscapingScopes does a pretty powerful escape analysis, which we might want to apply for other purposes in our HIR passes. This ports this pass to HIR. For the most part, this implementation is identical to the ReactiveFunction version. It now handles phis instead of conditional ReactiveExpressions, which it does by treating all the phi operands as possibly aliasing the lvalue. This also requires that we iterate the aliasing analysis to a fixpoint, because the HIR has backedges which the ReactiveFunctions don't. In our fixtures, this only changes one result, which appears to have become more accurate. I plan on testing this internally in a sync before landing. ghstack-source-id: 63aad5c97e8bd6ea58fb1363854d081f50a31fa3 Pull Request resolved: #31882
| while ((foo(), true)) { | ||
| x = (foo(), 2); |
There was a problem hiding this comment.
Did this fixture change due to the compiler now inferring x to be a primitive?
There was a problem hiding this comment.
Within this pass, yeah. Type inference was always precise and inferred x as a primitive, but the escape analysis pass was lumping in the rest of the values from the sequence expressions and seeing a function call plus an escaping value and thinking it had to memoize.
| const lvalues = [ | ||
| {place: value.lvalue.place, level: MemoizationLevel.Memoized}, | ||
| ]; | ||
| lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); |
There was a problem hiding this comment.
nit: could merge the push into the literal now that it's unconditional
| const lvalues = [ | ||
| {place: value.lvalue.place, level: MemoizationLevel.Unmemoized}, | ||
| ]; | ||
| lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); |
| const lvalues = [ | ||
| {place: value.lvalue, level: MemoizationLevel.Conditional}, | ||
| ]; | ||
| lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); |
| const rvalues = rvaluesUnfiltered.filter( | ||
| place => | ||
| !state.inScope( | ||
| block.instructions[0]?.id ?? block.terminal, |
There was a problem hiding this comment.
this is an instruction id, right? i'm surprised this passes, i thought you'd need block.terminal.id on the RHS
| if (rvalues.length !== rvaluesUnfiltered.length) { | ||
| undefinedIdentifiers = (): Array<Place> => | ||
| (undefinedIdentifiers?.() ?? []).concat( | ||
| rvaluesUnfiltered.filter(v => new Set(rvalues).has(v)), |
There was a problem hiding this comment.
This is O(n^2) since it's creating a new Set for each instance, let's cache the Set. Also, i'm curious why make undefinedIdentifiers a function rather than an array or set of places?
|
|
||
| function pruneScopes(fn: HIRFunction, state: Set<IdentifierId>): void { | ||
| const prunedScopes = new Set<ScopeId>(); | ||
| const reassignments = new Map<IdentifierId, Set<Identifier>>(); |
There was a problem hiding this comment.
the change to use IdentifierId instead of DeclarationId makes sense for most cases, but for reassignments it seems a bit suspect because the id value may not line up btw the reassignment and the FinishMemoize instruction.
to me what this points to is that we may not need this logic anymore, would be good to check if we can remove it (or at least add a TODO so we can come back to this)
| * If the manual memo was a useMemo that got inlined, iterate through | ||
| * all reassignments to the iife temporary to ensure they're memoized. | ||
| */ | ||
| decls = reassignments.get(value.decl.identifier.id) ?? [ |
There was a problem hiding this comment.
specifically the id may not match up here
Stack from ghstack (oldest at bottom):
Summary:
PruneNonEscapingScopes does a pretty powerful escape analysis, which we might want to apply for other purposes in our HIR passes. This ports this pass to HIR. For the most part, this implementation is identical to the ReactiveFunction version. It now handles phis instead of conditional ReactiveExpressions, which it does by treating all the phi operands as possibly aliasing the lvalue. This also requires that we iterate the aliasing analysis to a fixpoint, because the HIR has backedges which the ReactiveFunctions don't.
In our fixtures, this only changes one result, which appears to have become more accurate. I plan on testing this internally in a sync before landing.