[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: 38c6e24d436c2d508f0862a6c05b19288be44f7a 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: e254393a8d2e7065e2640167cb4f1edcf6e77fd0 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: cf58e1a8a5efeff374d81f28e23f8e9d774d7a34 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: 8d8d2821b7e0f01381b93f5b15b7682c9f1505a8 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: 2e1b02844d3a814dce094b7e3812df799e54343f 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: 2e1b02844d3a814dce094b7e3812df799e54343f Pull Request resolved: #30796

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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

josephsavona commented Aug 22, 2024 •

edited

Loading

vercel bot commented Aug 22, 2024 •

edited

Loading

josephsavona Aug 22, 2024

josephsavona Aug 23, 2024

josephsavona Aug 23, 2024

josephsavona Aug 23, 2024

josephsavona Aug 23, 2024

josephsavona commented Aug 23, 2024

[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

Conversation

josephsavona commented Aug 22, 2024 • edited Loading

vercel bot commented Aug 22, 2024 • edited Loading

josephsavona Aug 22, 2024

Choose a reason for hiding this comment

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

josephsavona Aug 23, 2024

Choose a reason for hiding this comment

josephsavona commented Aug 23, 2024

josephsavona commented Aug 22, 2024 •

edited

Loading

vercel bot commented Aug 22, 2024 •

edited

Loading