From e0a2926bc16902c06f16f3603cf9dfb4ed455b19 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 31 Jul 2024 16:43:54 -0700 Subject: [PATCH 1/2] [compiler] Promote temporaries when necessary to prevent codegen reordering over side-effectful operations [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 8 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 8 +- .../ReactiveScopes/PromoteUsedTemporaries.ts | 216 ++++++++++++++++++ .../bug-codegen-inline-iife.expect.md | 92 -------- .../compiler/bug-codegen-inline-iife.ts | 36 --- .../codegen-inline-iife-reassign.expect.md | 62 +++++ .../compiler/codegen-inline-iife-reassign.ts | 18 ++ .../codegen-inline-iife-storeprop.expect.md | 60 +++++ .../compiler/codegen-inline-iife-storeprop.ts | 18 ++ .../compiler/codegen-inline-iife.expect.md | 56 +++++ .../fixtures/compiler/codegen-inline-iife.ts | 16 ++ 11 files changed, 454 insertions(+), 136 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 6c313062744..786e8bc57a5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -431,17 +431,17 @@ function* runWithEnvironment( value: reactiveFunction, }); - promoteUsedTemporaries(reactiveFunction); + pruneUnusedLValues(reactiveFunction); yield log({ kind: 'reactive', - name: 'PromoteUsedTemporaries', + name: 'PruneUnusedLValues', value: reactiveFunction, }); - pruneUnusedLValues(reactiveFunction); + promoteUsedTemporaries(reactiveFunction); yield log({ kind: 'reactive', - name: 'PruneUnusedLValues', + name: 'PromoteUsedTemporaries', value: reactiveFunction, }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index b773192d573..1c5745f6f9b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -1198,7 +1198,7 @@ function codegenInstructionNullable( value = null; } else { lvalue = instr.value.lvalue.pattern; - let hasReasign = false; + let hasReassign = false; let hasDeclaration = false; for (const place of eachPatternOperand(lvalue)) { if ( @@ -1208,10 +1208,10 @@ function codegenInstructionNullable( cx.temp.set(place.identifier.id, null); } const isDeclared = cx.hasDeclared(place.identifier); - hasReasign ||= isDeclared; + hasReassign ||= isDeclared; hasDeclaration ||= !isDeclared; } - if (hasReasign && hasDeclaration) { + if (hasReassign && hasDeclaration) { CompilerError.invariant(false, { reason: 'Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations)', @@ -1219,7 +1219,7 @@ function codegenInstructionNullable( loc: instr.loc, suggestions: null, }); - } else if (hasReasign) { + } else if (hasReassign) { kind = InstructionKind.Reassign; } value = codegenPlaceToExpression(cx, instr.value.value); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts index 65b27fe639f..ffccaad61ac 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -14,13 +14,17 @@ import { Place, PrunedReactiveScopeBlock, ReactiveFunction, + ReactiveInstruction, ReactiveScopeBlock, + ReactiveTerminalStatement, ReactiveValue, ScopeId, + SpreadPattern, promoteTemporary, promoteTemporaryJsxTag, } from '../HIR/HIR'; import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; +import {eachInstructionValueLValue, eachPatternOperand} from '../HIR/visitors'; class Visitor extends ReactiveFunctionVisitor { override visitScope(scopeBlock: ReactiveScopeBlock, state: State): void { @@ -148,6 +152,212 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { } } +type InterState = Map; +class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { + /* + * Unpromoted temporaries will be emitted at their use sites rather than as separate + * declarations. However, this causes errors if an interposing temporary has been + * promoted, or if an interposing instruction has had its lvalues + */ + #promotable: State; + #consts: Set = new Set(); + + constructor(promotable: State, params: Array) { + super(); + params.forEach(param => { + switch (param.kind) { + case 'Identifier': + this.#consts.add(param.identifier.id); + break; + case 'Spread': + this.#consts.add(param.place.identifier.id); + break; + } + }); + this.#promotable = promotable; + } + + override visitParam(place: Place, _state: InterState): void { + this.#consts.add(place.identifier.id); + } + + override visitPlace( + _id: InstructionId, + place: Place, + state: InterState, + ): void { + const promo = state.get(place.identifier.id); + if (promo) { + const [identifier, needsPromotion] = promo; + if ( + needsPromotion && + identifier.name === null && + !this.#consts.has(identifier.id) + ) { + promoteIdentifier(identifier, this.#promotable); + } + } + } + + override visitInstruction( + instruction: ReactiveInstruction, + state: InterState, + ): void { + for (const lval of eachInstructionValueLValue(instruction.value)) { + CompilerError.invariant(lval.identifier.name != null, { + reason: + 'PromoteInterposedTemporaries: Assignment targets not expected to be temporaries', + loc: instruction.loc, + }); + } + + switch (instruction.value.kind) { + case 'DeclareContext': + case 'DeclareLocal': { + if ( + instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst' + ) { + this.#consts.add(instruction.value.lvalue.place.identifier.id); + } + super.visitInstruction(instruction, state); + break; + } + case 'CallExpression': + case 'MethodCall': + case 'Await': + case 'PropertyStore': + case 'PropertyDelete': + case 'ComputedStore': + case 'ComputedDelete': + case 'PostfixUpdate': + case 'PrefixUpdate': + case 'StoreLocal': + case 'StoreContext': + case 'StoreGlobal': + case 'Destructure': { + const preEntries = [...state.entries()]; + + if ( + (instruction.value.kind === 'StoreContext' || + instruction.value.kind === 'StoreLocal') && + (instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst') + ) { + this.#consts.add(instruction.value.lvalue.place.identifier.id); + } + if ( + instruction.value.kind === 'Destructure' && + (instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst') + ) { + [...eachPatternOperand(instruction.value.lvalue.pattern)].forEach( + ident => this.#consts.add(ident.identifier.id), + ); + } + if (instruction.value.kind === 'MethodCall') { + // Treat property of method call as constlike so we don't promote it. This is potentially unsound. + this.#consts.add(instruction.value.property.identifier.id); + } + + super.visitInstruction(instruction, state); + + if (instruction.lvalue && instruction.lvalue.identifier.name === null) { + state.set(instruction.lvalue.identifier.id, [ + instruction.lvalue.identifier, + false, + ]); + } + if ( + instruction.lvalue == null || + instruction.lvalue.identifier.name != null + ) { + // console.log(`marking at ${instruction.id}:`); + for (const [key, [ident, _]] of preEntries) { + // console.log(key); + state.set(key, [ident, true]); + } + } + break; + } + case 'LoadContext': + case 'LoadLocal': { + if (instruction.lvalue && instruction.lvalue.identifier.name === null) { + if (this.#consts.has(instruction.value.place.identifier.id)) { + this.#consts.add(instruction.lvalue.identifier.id); + } + state.set(instruction.lvalue.identifier.id, [ + instruction.lvalue.identifier, + false, + ]); + } + super.visitInstruction(instruction, state); + break; + } + case 'PropertyLoad': + case 'ComputedLoad': { + if (instruction.lvalue && instruction.lvalue.identifier.name === null) { + state.set(instruction.lvalue.identifier.id, [ + instruction.lvalue.identifier, + false, + ]); + } + super.visitInstruction(instruction, state); + break; + } + default: { + super.visitInstruction(instruction, state); + } + } + } + + override traverseTerminal( + stmt: ReactiveTerminalStatement, + state: InterState, + ): void { + function copyState(): InterState { + const map: InterState = new Map(); + state.forEach(([id, promo], key) => map.set(key, [id, promo])); + return map; + } + function mergeFrom(state2: InterState): void { + for (const [key, [ident, promo1]] of state2.entries()) { + const promo2 = state.get(key)?.[1] ?? false; + state.set(key, [ident, promo1 || promo2]); + } + } + + const {terminal} = stmt; + if (terminal.id !== null) { + this.visitID(terminal.id, state); + } + switch (terminal.kind) { + case 'if': { + this.visitPlace(terminal.id, terminal.test, state); + const state1 = copyState(); + this.visitBlock(terminal.consequent, state1); + if (terminal.alternate !== null) { + const state2 = copyState(); + this.visitBlock(terminal.alternate, state2); + mergeFrom(state2); + } + mergeFrom(state1); + break; + } + default: { + /* + * No special case for switch, try, etc because + * we don't know when running a later branch that + * any particular earlier branch will definitely not + * have executed. This means we err on the side of + * promotion. + */ + super.traverseTerminal(stmt, state); + } + } + } +} + export function promoteUsedTemporaries(fn: ReactiveFunction): void { const state: State = { tags: new Set(), @@ -161,6 +371,12 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void { } } visitReactiveFunction(fn, new Visitor(), state); + + visitReactiveFunction( + fn, + new PromoteInterposedTemporaries(state, fn.params), + new Map(), + ); } function promoteIdentifier(identifier: Identifier, state: State): void { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md deleted file mode 100644 index 4d36117ec72..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md +++ /dev/null @@ -1,92 +0,0 @@ - -## Input - -```javascript -import {makeArray, print} from 'shared-runtime'; - -/** - * Exposes bug involving iife inlining + codegen. - * We currently inline iifes to labeled blocks (not value-blocks). - * - * Here, print(1) and the evaluation of makeArray(...) get the same scope - * as the compiler infers that the makeArray call may mutate its arguments. - * Since print(1) does not get its own scope (and is thus not a declaration - * or dependency), it does not get promoted. - * As a result, print(1) gets reordered across the labeled-block instructions - * to be inlined at the makeArray callsite. - * - * Current evaluator results: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) [null,2] - * logs: [1,2] - * Forget: - * (kind: ok) [null,2] - * logs: [2,1] - */ -function useTest() { - return makeArray( - print(1), - (function foo() { - print(2); - return 2; - })(), - ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useTest, - params: [], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { makeArray, print } from "shared-runtime"; - -/** - * Exposes bug involving iife inlining + codegen. - * We currently inline iifes to labeled blocks (not value-blocks). - * - * Here, print(1) and the evaluation of makeArray(...) get the same scope - * as the compiler infers that the makeArray call may mutate its arguments. - * Since print(1) does not get its own scope (and is thus not a declaration - * or dependency), it does not get promoted. - * As a result, print(1) gets reordered across the labeled-block instructions - * to be inlined at the makeArray callsite. - * - * Current evaluator results: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) [null,2] - * logs: [1,2] - * Forget: - * (kind: ok) [null,2] - * logs: [2,1] - */ -function useTest() { - const $ = _c(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let t1; - - print(2); - t1 = 2; - t0 = makeArray(print(1), t1); - $[0] = t0; - } else { - t0 = $[0]; - } - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useTest, - params: [], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts deleted file mode 100644 index 0aa4a95bd52..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts +++ /dev/null @@ -1,36 +0,0 @@ -import {makeArray, print} from 'shared-runtime'; - -/** - * Exposes bug involving iife inlining + codegen. - * We currently inline iifes to labeled blocks (not value-blocks). - * - * Here, print(1) and the evaluation of makeArray(...) get the same scope - * as the compiler infers that the makeArray call may mutate its arguments. - * Since print(1) does not get its own scope (and is thus not a declaration - * or dependency), it does not get promoted. - * As a result, print(1) gets reordered across the labeled-block instructions - * to be inlined at the makeArray callsite. - * - * Current evaluator results: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok) [null,2] - * logs: [1,2] - * Forget: - * (kind: ok) [null,2] - * logs: [2,1] - */ -function useTest() { - return makeArray( - print(1), - (function foo() { - print(2); - return 2; - })(), - ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useTest, - params: [], -}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md new file mode 100644 index 00000000000..d17c934b3b4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md @@ -0,0 +1,62 @@ + +## Input + +```javascript +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w = 42), + w, + (function foo() { + w = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + let w; + w = {}; + + const t1 = (w = 42); + const t2 = w; + + w; + let t3; + w = 999; + t3 = 2; + t0 = makeArray(t1, t2, t3); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +### Eval output +(kind: ok) [42,42,2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts new file mode 100644 index 00000000000..8dd93a8482b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.ts @@ -0,0 +1,18 @@ +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w = 42), + w, + (function foo() { + w = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md new file mode 100644 index 00000000000..58c54ddaab8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.expect.md @@ -0,0 +1,60 @@ + +## Input + +```javascript +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w.x = 42), + w.x, + (function foo() { + w.x = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const w = {}; + + const t1 = (w.x = 42); + const t2 = w.x; + let t3; + + w.x = 999; + t3 = 2; + t0 = makeArray(t1, t2, t3); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +### Eval output +(kind: ok) [42,42,2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts new file mode 100644 index 00000000000..1b9f10856e4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-storeprop.ts @@ -0,0 +1,18 @@ +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + let w = {}; + return makeArray( + (w.x = 42), + w.x, + (function foo() { + w.x = 999; + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md new file mode 100644 index 00000000000..25a08bc3329 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + return makeArray( + print(1), + (function foo() { + print(2); + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeArray, print } from "shared-runtime"; + +function useTest() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const t1 = print(1); + let t2; + + print(2); + t2 = 2; + t0 = makeArray(t1, t2); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; + +``` + +### Eval output +(kind: ok) [null,2] +logs: [1,2] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts new file mode 100644 index 00000000000..52b1e8a9278 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife.ts @@ -0,0 +1,16 @@ +import {makeArray, print} from 'shared-runtime'; + +function useTest() { + return makeArray( + print(1), + (function foo() { + print(2); + return 2; + })(), + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useTest, + params: [], +}; From a16f79d4e6146b38495385113232e98b21f799cf Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 09:20:19 -0700 Subject: [PATCH 2/2] Update on "[compiler] Promote temporaries when necessary to prevent codegen reordering over side-effectful operations" [ghstack-poisoned] --- .../ReactiveScopes/PromoteUsedTemporaries.ts | 77 +++++++++++++------ 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts index ffccaad61ac..ac31bd469aa 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -154,14 +154,14 @@ class CollectPromotableTemporaries extends ReactiveFunctionVisitor { type InterState = Map; class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { + #promotable: State; + #consts: Set = new Set(); + /* * Unpromoted temporaries will be emitted at their use sites rather than as separate * declarations. However, this causes errors if an interposing temporary has been * promoted, or if an interposing instruction has had its lvalues */ - #promotable: State; - #consts: Set = new Set(); - constructor(promotable: State, params: Array) { super(); params.forEach(param => { @@ -177,10 +177,6 @@ class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { this.#promotable = promotable; } - override visitParam(place: Place, _state: InterState): void { - this.#consts.add(place.identifier.id); - } - override visitPlace( _id: InstructionId, place: Place, @@ -194,6 +190,11 @@ class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { identifier.name === null && !this.#consts.has(identifier.id) ) { + /* + * If the identifier hasn't been promoted but is marked as needing + * promotion by the logic in `visitInstruction`, and we've seen a + * use of it after said marking, promote it + */ promoteIdentifier(identifier, this.#promotable); } } @@ -212,17 +213,6 @@ class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { } switch (instruction.value.kind) { - case 'DeclareContext': - case 'DeclareLocal': { - if ( - instruction.value.lvalue.kind === 'Const' || - instruction.value.lvalue.kind === 'HoistedConst' - ) { - this.#consts.add(instruction.value.lvalue.place.identifier.id); - } - super.visitInstruction(instruction, state); - break; - } case 'CallExpression': case 'MethodCall': case 'Await': @@ -236,15 +226,27 @@ class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { case 'StoreContext': case 'StoreGlobal': case 'Destructure': { + /* + * Copy current state entries so that we can iterate over it + * later without hitting the entries added by visiting this + * instruction. + */ const preEntries = [...state.entries()]; + let constStore = false; + if ( (instruction.value.kind === 'StoreContext' || instruction.value.kind === 'StoreLocal') && (instruction.value.lvalue.kind === 'Const' || instruction.value.lvalue.kind === 'HoistedConst') ) { + /* + * If an identifier is const, we don't need to worry about it + * being mutated between being loaded and being used + */ this.#consts.add(instruction.value.lvalue.place.identifier.id); + constStore = true; } if ( instruction.value.kind === 'Destructure' && @@ -254,32 +256,54 @@ class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { [...eachPatternOperand(instruction.value.lvalue.pattern)].forEach( ident => this.#consts.add(ident.identifier.id), ); + constStore = true; } if (instruction.value.kind === 'MethodCall') { - // Treat property of method call as constlike so we don't promote it. This is potentially unsound. + // Treat property of method call as constlike so we don't promote it. this.#consts.add(instruction.value.property.identifier.id); } super.visitInstruction(instruction, state); if (instruction.lvalue && instruction.lvalue.identifier.name === null) { + // Add this instruction's lvalue to the state, initially not marked as needing promotion state.set(instruction.lvalue.identifier.id, [ instruction.lvalue.identifier, false, ]); } if ( - instruction.lvalue == null || - instruction.lvalue.identifier.name != null + !constStore && + (instruction.lvalue == null || + instruction.lvalue.identifier.name != null) ) { - // console.log(`marking at ${instruction.id}:`); + /* + * If we've stripped the lvalue or promoted the lvalue, then we will emit this instruction + * as a statement in codegen. + * + * If this instruction will be emitted directly as a statement rather than as a temporary + * during codegen, then it can interpose between the defs and the uses of other temporaries. + * Since this instruction could potentially mutate those defs, it's not safe to relocate + * the definition of those temporaries to after this instruction. Mark all those temporaries + * as needing promotion, but don't promote them until we actually see them being used. + */ for (const [key, [ident, _]] of preEntries) { - // console.log(key); state.set(key, [ident, true]); } } break; } + case 'DeclareContext': + case 'DeclareLocal': { + if ( + instruction.value.lvalue.kind === 'Const' || + instruction.value.lvalue.kind === 'HoistedConst' + ) { + this.#consts.add(instruction.value.lvalue.place.identifier.id); + } + super.visitInstruction(instruction, state); + break; + } case 'LoadContext': case 'LoadLocal': { if (instruction.lvalue && instruction.lvalue.identifier.name === null) { @@ -333,6 +357,13 @@ class PromoteInterposedTemporaries extends ReactiveFunctionVisitor { } switch (terminal.kind) { case 'if': { + /* + * Visit if branches separately -- we don't want + * to promote a use in the alternate branch because + * of an interposing instruction in the consequent branch, + * because it's not actually interposing at runtime. + * After finishing the branches, merge their states. + */ this.visitPlace(terminal.id, terminal.test, state); const state1 = copyState(); this.visitBlock(terminal.consequent, state1);