From d88f26a7e12bbd66e893789bef2b65600ffda128 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 12 Jul 2024 12:58:56 +0900 Subject: [PATCH 1/2] [compiler] Fix mode for generating scopes for reassignments We have an experimental mode where we generate scopes for simple phi values, even if they aren't subsequently mutated. This mode was incorrectly generating scope ranges, leaving the start at 0 which is invalid. The fix is to allow non-zero identifier ranges to overwrite the scope start (rather than taking the min) if the scope start is still zero. [ghstack-poisoned] --- .../InferReactiveScopeVariables.ts | 10 ++- ...signed-loop-force-scopes-enabled.expect.md | 68 +++++++++++++++++++ ...ve-reassigned-loop-force-scopes-enabled.js | 19 ++++++ 3 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 2c9e67646b155..10081b6ce413d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -114,9 +114,13 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { }; scopes.set(groupIdentifier, scope); } else { - scope.range.start = makeInstructionId( - Math.min(scope.range.start, identifier.mutableRange.start) - ); + if (identifier.mutableRange.start !== 0 && scope.range.start !== 0) { + scope.range.start = makeInstructionId( + Math.min(scope.range.start, identifier.mutableRange.start) + ); + } else if (identifier.mutableRange.start !== 0) { + scope.range.start = identifier.mutableRange.start; + } scope.range.end = makeInstructionId( Math.max(scope.range.end, identifier.mutableRange.end) ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.expect.md new file mode 100644 index 0000000000000..a40c7452bb973 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.expect.md @@ -0,0 +1,68 @@ + +## Input + +```javascript +// @enableForest +function Component({ base, start, increment, test }) { + let value = base; + for (let i = start; i < test; i += increment) { + value += i; + } + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ base: 0, start: 0, test: 10, increment: 1 }], + sequentialRenders: [ + { base: 0, start: 1, test: 10, increment: 1 }, + { base: 0, start: 0, test: 10, increment: 2 }, + { base: 2, start: 0, test: 10, increment: 2 }, + { base: 0, start: 0, test: 11, increment: 2 }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableForest +function Component(t0) { + const $ = _c(5); + const { base, start, increment, test } = t0; + let value; + if ($[0] !== base || $[1] !== start || $[2] !== test || $[3] !== increment) { + value = base; + for (let i = start; i < test; i = i + increment, i) { + value = value + i; + } + $[0] = base; + $[1] = start; + $[2] = test; + $[3] = increment; + $[4] = value; + } else { + value = $[4]; + } + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ base: 0, start: 0, test: 10, increment: 1 }], + sequentialRenders: [ + { base: 0, start: 1, test: 10, increment: 1 }, + { base: 0, start: 0, test: 10, increment: 2 }, + { base: 2, start: 0, test: 10, increment: 2 }, + { base: 0, start: 0, test: 11, increment: 2 }, + ], +}; + +``` + +### Eval output +(kind: ok)
45
+
20
+
22
+
30
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.js new file mode 100644 index 0000000000000..2cbf8ee58a8dd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/primitive-reassigned-loop-force-scopes-enabled.js @@ -0,0 +1,19 @@ +// @enableForest +function Component({ base, start, increment, test }) { + let value = base; + for (let i = start; i < test; i += increment) { + value += i; + } + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ base: 0, start: 0, test: 10, increment: 1 }], + sequentialRenders: [ + { base: 0, start: 1, test: 10, increment: 1 }, + { base: 0, start: 0, test: 10, increment: 2 }, + { base: 2, start: 0, test: 10, increment: 2 }, + { base: 0, start: 0, test: 11, increment: 2 }, + ], +}; From 3f6d0e48488324aeae14fb7c822bd1a76f45034c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 12 Jul 2024 13:00:51 +0900 Subject: [PATCH 2/2] Update on "[compiler] Fix mode for generating scopes for reassignments" We have an experimental mode where we generate scopes for simple phi values, even if they aren't subsequently mutated. This mode was incorrectly generating scope ranges, leaving the start at 0 which is invalid. The fix is to allow non-zero identifier ranges to overwrite the scope start (rather than taking the min) if the scope start is still zero. [ghstack-poisoned] --- .../src/ReactiveScopes/InferReactiveScopeVariables.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 10081b6ce413d..3e344d1bfa34c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -114,12 +114,12 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { }; scopes.set(groupIdentifier, scope); } else { - if (identifier.mutableRange.start !== 0 && scope.range.start !== 0) { + if (scope.range.start === 0) { + scope.range.start = identifier.mutableRange.start; + } else if (identifier.mutableRange.start !== 0) { scope.range.start = makeInstructionId( Math.min(scope.range.start, identifier.mutableRange.start) ); - } else if (identifier.mutableRange.start !== 0) { - scope.range.start = identifier.mutableRange.start; } scope.range.end = makeInstructionId( Math.max(scope.range.end, identifier.mutableRange.end)