[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
| if ( | ||
| (instr.value.kind === 'FunctionExpression' || | ||
| instr.value.kind === 'ObjectMethod') && | ||
| place.identifier.type.kind === 'Primitive' | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Can you double-check this case a bit more? Every time we have to special case something like this there's a chance that we're overlooking the larger rule. Why do we need to skip primitives, and why only for functionexpr/objectmethod?
If nothing else, this repeatedly checks properties of the instruction value for every single operand, which is inefficient, but i'm more concerned with the check itself.
| const context = instrValue.loweredFunc.func.context | ||
| .map(dep => printPlace(dep)) | ||
| .map(dep => `${printPlace(dep)}`) |
There was a problem hiding this comment.
nit: the original printPlace(dep) is more clear
| CompilerError.invariant(operand.effect === Effect.Unknown, { | ||
| reason: 'Unexpected unknown effect', |
There was a problem hiding this comment.
we expect the effect to be unknown in the condition, but the error message says the opposite (?)
| operandValues.size === 1 && | ||
| operandValues.values().next().value?.kind === 'DeclareContext' |
There was a problem hiding this comment.
hmm why only consider hoisted context variables if there is one operand? could you take the example below and also add some other context reference and break this?
| if ( | ||
| instr.value.kind === 'FunctionExpression' || | ||
| instr.value.kind === 'ObjectMethod' | ||
| ) { | ||
| if (operand.identifier.type.kind === 'Primitive') { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
see comment above, i'd like to understand this more. why this exact combination and not primitives of other instruction types, for example.
| if (DEBUG) { | ||
| for (const [, block] of ir.blocks) { | ||
| for (const phi of block.phis) { | ||
| CompilerError.invariant(!rewrites.has(phi.place.identifier), { | ||
| reason: '[EliminateRedundantPhis]: rewrite not complete', | ||
| loc: phi.place.loc, | ||
| }); | ||
| for (const [, operand] of phi.operands) { | ||
| CompilerError.invariant(!rewrites.has(operand.identifier), { | ||
| reason: '[EliminateRedundantPhis]: rewrite not complete', | ||
| loc: phi.place.loc, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
revert this file? or set DEBUG=false
| @@ -27,7 +27,6 @@ export const FIXTURE_ENTRYPOINT = { | |||
| import { c as _c } from "react/compiler-runtime"; | |||
| function component(a, b) { | |||
| const $ = _c(2); | |||
| const y = { b }; | |||
There was a problem hiding this comment.
I wondered about the exact mechanism that was kicking in to let us prune this. We have always had code that recursively calls DCE on inner functions (its part of AnalyzeFunctions). And DCE prunes unused context variables on the outermost function its called on. So we were pruning the context access for y. But because we didn't prune unused deps, the outer DCE thought that y was still used. Now the inner DCE removes y from context, and then the outer DCE can see that y is unused and prune it.
There was a problem hiding this comment.
Yep that's completely right. I thought about writing a separate pass to prune unused deps and their source instructions to reduce noise of the snapshot diffing for this PR. Ultimately it didn't seem worth the time
compiler/scripts/bundle-meta.sh
Outdated
There was a problem hiding this comment.
what's this doing here?
Small patch to pass aliased context values into `Object|ArrayExpression`s --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32093). * #32099 * #32104 * #32098 * #32097 * #32096 * #32095 * #32094 * __->__ #32093
See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32094). * #32099 * #32104 * #32098 * #32097 * #32096 * #32095 * __->__ #32094 * #32093
See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32095). * #32099 * #32104 * #32098 * #32097 * #32096 * __->__ #32095 * #32094 * #32093
LoweredFunction dependencies were exclusively used for dependency extraction (in `propagateScopeDeps`). Now that we have a `propagateScopeDepsHIR` that recursively traverses into nested functions, we can delete `dependencies` and their associated synthetic `LoadLocal`/`PropertyLoad` instructions.
LoweredFunction dependencies were exclusively used for dependency extraction (in
propagateScopeDeps). Now that we have apropagateScopeDepsHIRthat recursively traverses into nested functions, we can deletedependenciesand their associated syntheticLoadLocal/PropertyLoadinstructions.Stack created with Sapling. Best reviewed with ReviewStack.