[compiler] Delete LoweredFunction.dependencies and hoisted instructions #31204
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 11, 2024
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
This was referenced Oct 24, 2024
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects: - optional-chaining deps for inner functions - full DCE and outlining for inner functions - fewer extraneous instructions
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd)
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) -
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Oct 24, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions '
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
This bugfix is needed to land #31199 PropagateScopeDepsHIR infers scope declarations for the `inline-jsx-transform` test fixture (the non-hir version does not). These declarations must get the rewritten phi identifiers
`enablePropagateScopeDepsHIR` is now used extensively in Meta. This has been tested for over two weeks in our e2e tests and production. The rest of this stack deletes `LoweredFunction.dependencies`, which the non-hir version of `PropagateScopeDeps` depends on. To avoid a more forked HIR (non-hir with dependencies and hir with no dependencies), let's go ahead and clean up the non-hir version of PropagateScopeDepsHIR. Note that all fixture changes in this PR were previously reviewed when they were copied to `propagate-scope-deps-hir-fork`. Will clean up / merge these duplicate fixtures in a later PR '
Recursively collect identifier / property loads and optional chains from inner functions. This PR is in preparation for #31200 Previously, we only did this in `collectHoistablePropertyLoads` to understand hoistable property loads from inner functions. 1. collectTemporariesSidemap 2. collectOptionalChainSidemap 3. collectHoistablePropertyLoads - ^ this recursively calls `collectTemporariesSidemap`, `collectOptionalChainSidemap`, and `collectOptionalChainSidemap` on inner functions 4. collectDependencies Now, we have 1. collectTemporariesSidemap - recursively record identifiers in inner functions. Note that we track all temporaries in the same map as `IdentifierIds` are currently unique across functions 2. collectOptionalChainSidemap - recursively records optional chain sidemaps in inner functions 3. collectHoistablePropertyLoads - (unchanged, except to remove recursive collection of temporaries) 4. collectDependencies - unchanged: to be modified to recursively collect dependencies in next PR '
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
`JSXMemberExpression` is currently the only instruction (that I know of) that directly references identifier lvalues without a corresponding `LoadLocal`. This has some side effects: - deadcode elimination and constant propagation now reach JSXMemberExpressions - we can delete `LoweredFunction.dependencies` without dangling references (previously, the only reference to JSXMemberExpression objects in HIR was in function dependencies) - JSXMemberExpression now is consistent with all other instructions (e.g. has a rvalue-producing LoadLocal) '
Add more `FIXTURE_ENTRYPOINT`s '
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
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 artificial `LoadLocal`/`PropertyLoad` instructions. '
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Extends #31066 to ObjectMethods (somehow missed this before). ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31197). * #31204 * #31202 * #31203 * #31201 * #31200 * #31346 * #31199 * #31431 * #31345 * __->__ #31197
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31345). * #31204 * #31202 * #31203 * #31201 * #31200 * #31346 * #31199 * #31431 * __->__ #31345 * #31197
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
This bugfix is needed to land #31199 PropagateScopeDepsHIR infers scope declarations for the `inline-jsx-transform` test fixture (the non-hir version does not). These declarations must get the rewritten phi identifiers --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31431). * #31204 * #31202 * #31203 * #31201 * #31200 * #31346 * #31199 * __->__ #31431 * #31345 * #31197
|
Discussed offline with @josephsavona and @mvitousek. I'm temporarily closing this PR as it requires non-trivial changes to how we currently model effects. |
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) '
mofeiZ
added a commit
that referenced
this pull request
Nov 5, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
mofeiZ
added a commit
that referenced
this pull request
Nov 12, 2024
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 artificialLoadLocal/PropertyLoadinstructions.'
Stack created with Sapling. Best reviewed with ReviewStack.
dependenciesin propagateScopeDeps #31200