Skip to content

[compiler] Prepare HIRBuilder to be used by later passes #32286

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.

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

Merged
merged 1 commit into from
May 22, 2025
Merged

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Feb 1, 2025

@josephsavona
Copy link
Member

josephsavona commented Feb 2, 2025

This one I’m really curious about. We’ve never needed to use HIRBuilder after initial HIR construction, let’s chat to make sure we’re aligned!

@mofeiZ
Copy link
Contributor Author

mofeiZ commented Feb 3, 2025

This one I’m really curious about. We’ve never needed to use HIRBuilder after initial HIR construction, let’s chat to make sure we’re aligned!

Happy to do so! This PR comes from splitting up #32099 and is needed for scope dependency instruction construction

@mofeiZ mofeiZ marked this pull request as ready for review February 14, 2025 22:26
mofeiZ added a commit that referenced this pull request Feb 18, 2025
…ns (#32096)

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.

[Internal snapshot
diff](https://www.internalfb.com/phabricator/paste/view/P1716950202) for
this change shows ~.2% of files changed. I [read through ~60 of the
changed
files](https://www.internalfb.com/phabricator/paste/view/P1733074307)
- most changes are due to better outlining (due to better DCE)
- a few changes in memo inference are due to changed ordering
```
// source
arr.map(() => contextVar.inner);

// previous instructions
$0 = LoadLocal arr
$1 = $0.map
// Below instructions are synthetic
$2 = LoadLocal contextVar
$3 = $2.inner
$4 = Function deps=$3 context=contextVar {
  ...
}
```
- a few changes are effectively bugfixes (see
`aliased-nested-scope-fn-expr`)
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32096).
* #32099
* #32286
* #32104
* #32098
* #32097
* __->__ #32096
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.

Cool! Still curious how this gets used but the changes themselves are all fine :-)

@mofeiZ
Copy link
Contributor Author

mofeiZ commented May 21, 2025

Cool! Still curious how this gets used but the changes themselves are all fine :-)

Thanks for the review! This gets used in #33326 to reserve optional block consequent and continuations

@mofeiZ mofeiZ merged commit 13f2004 into main May 22, 2025
20 of 29 checks passed
mofeiZ added a commit that referenced this pull request May 22, 2025
When collecting scope dependencies, mark each dependency with `reactive:
true | false`. This prepares for later PRs
#33326 and
#32099 which rewrite scope
dependencies into instructions.

Note that some reactive objects may have non-reactive properties, but we
do not currently track this.

Technically, state[0] is reactive and state[1] is not. Currently, both
would be marked as reactive.
```js
const state = useState();
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33325).
* #33326
* __->__ #33325
* #32286
mofeiZ added a commit that referenced this pull request May 22, 2025
…33326)

Inferred effect dependencies now include optional chains.

This is a temporary solution while
#32099 and its followups are
worked on. Ideally, we should model reactive scope dependencies in the
IR similarly to `ComputeIR` -- dependencies should be hoisted and all
references rewritten to use the hoisted dependencies.

`
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33326).
* __->__ #33326
* #33325
* #32286
github-actions bot pushed a commit that referenced this pull request May 22, 2025
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32286).
* #33326
* #33325
* __->__ #32286

DiffTrain build for [13f2004](13f2004)
github-actions bot pushed a commit that referenced this pull request May 22, 2025
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32286).
* #33326
* #33325
* __->__ #32286

DiffTrain build for [13f2004](13f2004)
github-actions bot pushed a commit that referenced this pull request May 22, 2025
…33326)

Inferred effect dependencies now include optional chains.

This is a temporary solution while
#32099 and its followups are
worked on. Ideally, we should model reactive scope dependencies in the
IR similarly to `ComputeIR` -- dependencies should be hoisted and all
references rewritten to use the hoisted dependencies.

`
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33326).
* __->__ #33326
* #33325
* #32286

DiffTrain build for [0d07288](0d07288)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants