[compiler] Infer phi types, extend mutable ranges to account for Store effects #30796

josephsavona · 2024-08-22T23:52:05Z

Stack from ghstack (oldest at bottom):

Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this:

Update InferTypes to infer the type of phi.id.type, not the unused phi.type.
Update the algorithm to verify that all the phi types are actually equal, not just have the same kind.
Handle circular types by removing the cycle.

However, that reveals another issue: InferMutableRanges currently infers the results of Store effects after its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. So the PR also updates InferMutableRanges to add a second fixpoint loop to extend the range of all direct aliases of values mutated via store effects. See code comments for the thinking here.

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. [ghstack-poisoned]

vercel · 2024-08-22T23:52:06Z

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 23, 2024 0:29am

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: 38c6e24 Pull Request resolved: #30796

…nt for Store effects" Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. [ghstack-poisoned]

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: e254393 Pull Request resolved: #30796

josephsavona · 2024-08-22T23:54:02Z

compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts

+      let candidateType: Type | null = null;
+      for (const operand of type.operands) {
+        const resolved = this.get(operand);
+        if (candidateType === null) {
+          candidateType = resolved;
+        } else if (!typeEquals(resolved, candidateType)) {
+          candidateType = null;
+          break;
+        } // else same type, continue
+      }
+
+      if (candidateType !== null) {
+        this.unify(v, candidateType);
        return;
      }


the previous logic was incorrect, checking only that the operand type kinds matched. we need to check that the types actually match

…nt for Store effects" Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. [ghstack-poisoned]

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: cf58e1a Pull Request resolved: #30796

...ages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-push-effect.expect.md

…nt for Store effects" Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. [ghstack-poisoned]

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: 8d8d282 Pull Request resolved: #30796

josephsavona · 2024-08-23T00:19:53Z

...n-react-compiler/src/__tests__/fixtures/compiler/phi-type-inference-property-store.expect.md

-  }
-  let t1;
-  if ($[3] !== y) {
+
    t1 = [x, y];


We now know the type of y and that type always invalidates, which makes these two scopes eligible for merging.

josephsavona · 2024-08-23T00:20:50Z

...lugin-react-compiler/src/__tests__/fixtures/compiler/phi-type-inference-array-push.expect.md

+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = {};
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  const x = t0;


we're now able to memoize x independently

josephsavona · 2024-08-23T00:21:39Z

...ler/src/__tests__/fixtures/compiler/phi-type-inference-array-push-consecutive-phis.expect.md

+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = {};
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  const x = t0;


we independently memoize x

josephsavona · 2024-08-23T00:22:02Z

...ler/src/__tests__/fixtures/compiler/phi-type-inference-array-push-consecutive-phis.expect.md

+    let y;
+    if (props.cond) {
+      if (props.cond2) {
+        y = [props.value];
+      } else {
+        y = [props.value2];
+      }
+    } else {
+      y = [];
+    }
+
+    y.push(x);


while recognizing that all of these arrays being assigned to y are mutated at the y.push(x)

…nt for Store effects" Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. [ghstack-poisoned]

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: 2e1b028 Pull Request resolved: #30796

josephsavona · 2024-08-23T04:45:16Z

Closes #29907

…e effects Redo of an earlier (pre-OSS) PR to infer types of phi nodes. There are a few pieces to this: 1. Update InferTypes to infer the type of `phi.id.type`, not the unused `phi.type`. 2. Update the algorithm to verify that all the phi types are actually equal, not just have the same kind. 3. Handle circular types by removing the cycle. However, that reveals another issue: InferMutableRanges currently infers the results of `Store` effects _after_ its fixpoint loop. That was fine when a Store could never occur on a phi (since they wouldn't have a type to get a function signature from). Now though, we can have Store effects occur on phis, and we need to ensure that this correctly updates the mutable range of the phi operands - recursively. See new test that fails without the fixpoint loop. ghstack-source-id: 2e1b028 Pull Request resolved: #30796

Inferred effect dependencies and inlined jsx (both experimental features) rely on `InferReactivePlaces` to determine their dependencies. Since adding type inference for phi nodes (#30796), we have been incorrectly inferring stable-typed value blocks (e.g. `props.cond ? setState1 : setState2`) as non-reactive. This fix patches InferReactivePlaces instead of adding a new pass since we want non-reactivity propagated correctly

…32991) Inferred effect dependencies and inlined jsx (both experimental features) rely on `InferReactivePlaces` to determine their dependencies. Since adding type inference for phi nodes (#30796), we have been incorrectly inferring stable-typed value blocks (e.g. `props.cond ? setState1 : setState2`) as non-reactive. This fix patches InferReactivePlaces instead of adding a new pass since we want non-reactivity propagated correctly

…32991) Inferred effect dependencies and inlined jsx (both experimental features) rely on `InferReactivePlaces` to determine their dependencies. Since adding type inference for phi nodes (#30796), we have been incorrectly inferring stable-typed value blocks (e.g. `props.cond ? setState1 : setState2`) as non-reactive. This fix patches InferReactivePlaces instead of adding a new pass since we want non-reactivity propagated correctly DiffTrain build for [2d0a5e3](2d0a5e3)

This was referenced Aug 22, 2024

[compiler] Repros for missing memoization due to lack of phi type inference #30793

Merged

[compiler] Typedefs for Array.prototype.flatMap #30794

Merged

[compiler] Remove phi type, infer phi.id.type #30795

Merged

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

josephsavona commented Aug 22, 2024

View reviewed changes

vercel bot deployed to Preview August 23, 2024 00:04 View deployment

josephsavona commented Aug 23, 2024

View reviewed changes

...ages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-push-effect.expect.md Outdated Show resolved Hide resolved

josephsavona commented Aug 23, 2024

View reviewed changes

vercel bot deployed to Preview August 23, 2024 00:23 View deployment

vercel bot deployed to Preview August 23, 2024 00:29 View deployment

josephsavona mentioned this pull request Aug 23, 2024

[compiler] Special-case phi inference for mixed readonly type #30797

Merged

gsathya approved these changes Aug 23, 2024

View reviewed changes

josephsavona mentioned this pull request Aug 23, 2024

[wip] Early exploration of optional deps #30801

Closed

josephsavona merged commit f450193 into gh/josephsavona/44/base Aug 23, 2024
19 checks passed

josephsavona deleted the gh/josephsavona/44/head branch August 23, 2024 22:25

poteto mentioned this pull request Nov 13, 2024

[Compiler]: Type inference information does not propagate past phi nodes #29907

Closed

4 tasks

mofeiZ mentioned this pull request Apr 22, 2025

[compiler] Patch for reactive refs in inferred effect dependencies #32991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[compiler] Infer phi types, extend mutable ranges to account for Store effects #30796

[compiler] Infer phi types, extend mutable ranges to account for Store effects #30796

Uh oh!

josephsavona commented Aug 22, 2024 •

edited

Loading

Uh oh!

vercel bot commented Aug 22, 2024 •

edited

Loading

Uh oh!

josephsavona Aug 22, 2024

Uh oh!

Uh oh!

josephsavona Aug 23, 2024

Uh oh!

josephsavona Aug 23, 2024

Uh oh!

josephsavona Aug 23, 2024

Uh oh!

josephsavona Aug 23, 2024

Uh oh!

josephsavona commented Aug 23, 2024

Uh oh!

Uh oh!

Uh oh!

[compiler] Infer phi types, extend mutable ranges to account for Store effects #30796

[compiler] Infer phi types, extend mutable ranges to account for Store effects #30796

Uh oh!

Conversation

josephsavona commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsavona Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona commented Aug 23, 2024

Uh oh!

Uh oh!

Uh oh!

josephsavona commented Aug 22, 2024 •

edited

Loading

vercel bot commented Aug 22, 2024 •

edited

Loading