[compiler] Add fallthrough to branch terminal #30814

josephsavona · 2024-08-26T19:20:21Z

Stack from ghstack (oldest at bottom):

Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, foo?.bar?.baz creates terminals along the lines of:

bb0:
  optional fallthrough=bb4
bb1:
  optional fallthrough=bb3
bb2:
  ...
  branch ... (fallthrough=bb3)

...

bb3:
  ...
  branch ... (fallthrough=bb4)

...

bb4:
  ...

Without a fallthrough on branch terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up.

Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, `foo?.bar?.baz` creates terminals along the lines of: ``` bb0: optional fallthrough=bb4 bb1: optional fallthrough=bb3 bb2: ... branch ... (fallthrough=bb3) ... bb3: ... branch ... (fallthrough=bb4) ... bb4: ... ``` Without a fallthrough on `branch` terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up. [ghstack-poisoned]

vercel · 2024-08-26T19:20:23Z

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	Aug 28, 2024 6:12pm

Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, `foo?.bar?.baz` creates terminals along the lines of: ``` bb0: optional fallthrough=bb4 bb1: optional fallthrough=bb3 bb2: ... branch ... (fallthrough=bb3) ... bb3: ... branch ... (fallthrough=bb4) ... bb4: ... ``` Without a fallthrough on `branch` terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up. [ghstack-poisoned]

mofeiZ · 2024-08-28T19:24:13Z

compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts

+          case 'logical':
+          case 'sequence':
+          case 'ternary': {


Ah I see, I'm guessing that this is to support something like useMemo(..., [(a ?? b)?.c]). Not technically necessary for DropManualMemo as we're only tracking named variables, but this makes sense to have in a utility function

yup exactly

mofeiZ · 2024-08-28T19:37:49Z

compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts

+              if (last !== undefined && last.value.kind === 'StoreLocal') {
+                optionals.add(last.value.value.identifier.id);
+              }


This feels slightly weird since DropManualMemo runs before we enter ssa, so both consequent (the optional read) and alternate (undefined) write to the same identifier ID. There is also no phi join, so reads to the OptionalExpression rvalue is also guaranteed to use the same identifier ID (and this works)

(I'm guessing that passes after SSA will need to track phis as well -- reading the rest of the stack now)

Yeah I looked at using the phi to figure out which values is optional. First, as you noted we aren't in SSA so we don't have phis yet. Second, the phis end up in weird places for nested optionals, because with nested optionals the alternate actually goes to the outermost fallthrough. So we just look at where the consequent assigns to in order to figure out what the optional value was.

Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, `foo?.bar?.baz` creates terminals along the lines of: ``` bb0: optional fallthrough=bb4 bb1: optional fallthrough=bb3 bb2: ... branch ... (fallthrough=bb3) ... bb3: ... branch ... (fallthrough=bb4) ... bb4: ... ``` Without a fallthrough on `branch` terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up. ghstack-source-id: d48c6232899864716eef71798a278b487d30eafc Pull Request resolved: #30814

Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, `foo?.bar?.baz` creates terminals along the lines of: ``` bb0: optional fallthrough=bb4 bb1: optional fallthrough=bb3 bb2: ... branch ... (fallthrough=bb3) ... bb3: ... branch ... (fallthrough=bb4) ... bb4: ... ``` Without a fallthrough on `branch` terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up. ghstack-source-id: 064178d377e1eb8a308f4d2f0d3f0132817a5b56 Pull Request resolved: facebook/react#30814

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 26, 2024

vercel bot deployed to Preview August 26, 2024 19:25 View deployment

josephsavona mentioned this pull request Aug 26, 2024

[compiler] Infer optional dependencies (behind feature flag) #30819

Merged

vercel bot deployed to Preview August 27, 2024 05:43 View deployment

josephsavona mentioned this pull request Aug 28, 2024

[compiler] Handle optional where innermost property access is non-optional #30836

Merged

josephsavona added 2 commits August 28, 2024 10:44

vercel bot deployed to Preview August 28, 2024 18:12 View deployment

josephsavona mentioned this pull request Aug 28, 2024

[compiler] Enable optional dependencies by default #30838

Merged

mofeiZ reviewed Aug 28, 2024

View reviewed changes

mofeiZ approved these changes Aug 28, 2024

View reviewed changes

josephsavona mentioned this pull request Aug 28, 2024

[compiler] Inferred deps must match exact optionality of manual deps #30840

Merged

josephsavona merged commit 00d44be into gh/josephsavona/50/base Aug 28, 2024
19 checks passed

josephsavona deleted the gh/josephsavona/50/head branch August 28, 2024 22:59

This was referenced Sep 12, 2024

Upgrade React from 7771d3a7-20240827 to 47352209-20240912 vercel/next.js#70035

Closed

Upgrade React from 7771d3a7-20240827 to 94e652d5-20240912 vercel/next.js#70037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Add fallthrough to branch terminal #30814

[compiler] Add fallthrough to branch terminal #30814

josephsavona commented Aug 26, 2024 •

edited

Loading

vercel bot commented Aug 26, 2024 •

edited

Loading

mofeiZ Aug 28, 2024

josephsavona Aug 28, 2024

mofeiZ Aug 28, 2024

josephsavona Aug 28, 2024

[compiler] Add fallthrough to branch terminal #30814

[compiler] Add fallthrough to branch terminal #30814

Conversation

josephsavona commented Aug 26, 2024 • edited Loading

vercel bot commented Aug 26, 2024 • edited Loading

mofeiZ Aug 28, 2024

Choose a reason for hiding this comment

josephsavona Aug 28, 2024

Choose a reason for hiding this comment

mofeiZ Aug 28, 2024

Choose a reason for hiding this comment

josephsavona Aug 28, 2024

Choose a reason for hiding this comment

josephsavona commented Aug 26, 2024 •

edited

Loading

vercel bot commented Aug 26, 2024 •

edited

Loading