[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: 63aad5c 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
There was a problem hiding this comment.
Ugh, this is because the LHS of the ?? is not considered nullable, so the RHS is ignored. Fixed!
| 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?
There was a problem hiding this comment.
We're only generating the undefinedIdentifiers result if we hit a compiler error, so this defers computing it until we see an error. Probably a premature optimization tho.
|
|
||
| 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
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]
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: 03c5805 Pull Request resolved: #31882
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]
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: 4f6a7e6 Pull Request resolved: #31882
|
@josephsavona Resurrecting this diff! Updated to address your comments from January (using declarationId again) and integrating your changes from #33062. Internal diff TK |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
github-actions
bot
added
the
Resolution: Stale
|
Oh whoops we lost track of this |
github-actions
bot
removed
the
Resolution: Stale
4 participants
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.