Skip to content

[compiler][be] Clean up nested function context in DCE #31202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Nov 15, 2024
Merged

[compiler][be] Clean up nested function context in DCE #31202

merged 5 commits into from
Nov 15, 2024

Conversation

@mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Oct 11, 2024 •
edited
Loading

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)

'

Stack created with Sapling. Best reviewed with ReviewStack.

@vercel
Copy link

vercel bot commented Oct 11, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 5:08pm

This was referenced Oct 11, 2024
Merged
Merged
Merged
Merged
Merged
Closed
Closed
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 11, 2024
josephsavona
josephsavona reviewed Oct 11, 2024
View reviewed changes
Copy link
Member

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

niiiice

This was referenced Oct 24, 2024
Merged
Merged
mofeiZ added a commit that referenced this pull request Oct 24, 2024
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
a84a59d 
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
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
c5b7421 
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 mofeiZ marked this pull request as ready for review October 24, 2024 19:19
mofeiZ added a commit that referenced this pull request Oct 24, 2024
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
754d933 
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
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
a19e4bb 
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 mofeiZ mentioned this pull request Nov 5, 2024
Merged
mofeiZ added a commit that referenced this pull request Nov 5, 2024
@mofeiZ
[compiler][ez] Patch hoistability for ObjectMethods (#31197)
dd1a021 
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
@mofeiZ
[compiler] bugfix for hoistable deps for nested functions (#31345)
e7e269b 
`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
@mofeiZ
[compiler] patch: rewrite scope dep/decl in inlineJsxTransform (#31431)
527bcaa 
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
mofeiZ added a commit that referenced this pull request Nov 5, 2024
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
12fe556 
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
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
e8a7ef6 
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 6, 2024
@mofeiZ
[compiler] Delete propagateScopeDeps (non-hir) (#31199)
fd018af 
`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

'
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31199).
* #31202
* #31203
* #31201
* #31200
* #31346
* __->__ #31199
mofeiZ added a commit that referenced this pull request Nov 6, 2024
@mofeiZ
[compiler] Collect temporaries and optional chains from inner functio…
c3570b1 
…ns (#31346)

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

'
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31346).
* #31202
* #31203
* #31201
* #31200
* __->__ #31346
* #31199
josephsavona
josephsavona approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!!!!!

mofeiZ added a commit that referenced this pull request Nov 12, 2024
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
7812226 
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
[compiler] Fix: ref.current now correctly reactive
125581e 
We were previously filtering out `ref.current` dependencies in propagateScopeDependencies:checkValidDependency`. This is incorrect.

Instead, we now always take a dependency on ref values (the outer box) as they may be reactive. Pruning is done in pruneNonReactiveDependencies.

This PR includes a small patch to `collectReactiveIdentifier`. Prior to this, we conservatively assumed that pruned scopes always produced reactive declarations. This assumption fixed a bug with non-reactivity, but some of these declarations are `useRef` calls. Now we have special handling for this case
```js
// This often produces a pruned scope
React.useRef(1);
```
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps
6483117 
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
[compiler] Lower JSXMemberExpression with LoadLocal
6a143e5 
`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)

'
@mofeiZ
[compiler][be] Patch test fixtures for evaluator
83a96d3 
Add more `FIXTURE_ENTRYPOINT`s

'
@mofeiZ
[compiler][be] Clean up nested function context in DCE
6617907 
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 mofeiZ mentioned this pull request Nov 15, 2024
Merged
mofeiZ added a commit that referenced this pull request Nov 15, 2024
@mofeiZ
[compiler] Fix: ref.current now correctly reactive (#31521)
4972718 
We were previously filtering out `ref.current` dependencies in
propagateScopeDependencies:checkValidDependency`. This is incorrect.

Instead, we now always take a dependency on ref values (the outer box)
as they may be reactive. Pruning is done in
pruneNonReactiveDependencies.

This PR includes a small patch to `collectReactiveIdentifier`. Prior to
this, we conservatively assumed that pruned scopes always produced
reactive declarations. This assumption fixed a bug with non-reactivity,
but some of these declarations are `useRef` calls. Now we have special
handling for this case
```js
// This often produces a pruned scope
React.useRef(1);
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31521).
* #31202
* #31203
* #31201
* #31200
* __->__ #31521
mofeiZ added a commit that referenced this pull request Nov 15, 2024
@mofeiZ
[compiler] Stop using function dependencies in propagateScopeDeps (#…
c09402a 
…31200)

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)

-
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31200).
* #31202
* #31203
* #31201
* __->__ #31200
* #31521
mofeiZ added a commit that referenced this pull request Nov 15, 2024
@mofeiZ
[compiler] Lower JSXMemberExpression with LoadLocal (#31201)
858633f 
`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)

'
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31201).
* #31202
* #31203
* __->__ #31201
* #31200
* #31521
mofeiZ added a commit that referenced this pull request Nov 15, 2024
@mofeiZ
[compiler][be] Patch test fixtures for evaluator (#31203)
0f3c62b 
Add more `FIXTURE_ENTRYPOINT`s

'
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31203).
* #31202
* __->__ #31203
* #31201
* #31200
* #31521
@mofeiZ mofeiZ merged commit 0480cdb into main Nov 15, 2024
19 of 23 checks passed
@poteto poteto deleted the pr31202 branch November 18, 2024 15:18
@vercel-release-bot vercel-release-bot mentioned this pull request Nov 20, 2024
Closed
This was referenced Dec 4, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@josephsavona josephsavona josephsavona approved these changes

Assignees

No one assigned

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mofeiZ @josephsavona @facebook-github-bot