From 26cd435c7d45bed9075b48d6761f6f7874c7bdce Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 3 Jan 2023 14:11:04 -0800 Subject: [PATCH] Support chained assignment expressions Fixes codegen for chained assignment expressions. Previously each intermediate assignment would be generated independently _in addition_ to the final chained expression being emitted. We now emit a single chained expression, almost exactly matching the input except for expanding from `x += 1` into `x = x + 1`. There are two key changes: * Ensuring that assignment expressions always generate an lvalue, which is necessary for alias analysis to kick in, since it relies on the effect of the lvalue to know where to look for aliasing. * The above makes codegen think the entire assignment expression value is a temporary that can be emitted later, but that isn't true. The new PruneTemporaryLValue pass nulls out lvalues that are never read later, ensuring that codegen can eagerly emit the value instead of saving it as a temporary. --- compiler/forget/src/CompilerPipeline.ts | 4 + compiler/forget/src/HIR/BuildHIR.ts | 168 ++++++----------- compiler/forget/src/HIR/Codegen.ts | 9 +- .../ReactiveScopes/PruneTemporaryLValues.ts | 41 +++++ compiler/forget/src/ReactiveScopes/index.ts | 1 + .../forget/src/ReactiveScopes/visitors.ts | 170 +++++++++++++++++- ...g_chained-assignment-expressions.expect.md | 28 --- .../_bug_chained-assignment-expressions.js | 5 - .../chained-assignment-expressions.expect.md | 35 ++++ .../hir/chained-assignment-expressions.js | 8 + 10 files changed, 309 insertions(+), 160 deletions(-) create mode 100644 compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts delete mode 100644 compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md delete mode 100644 compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js create mode 100644 compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js diff --git a/compiler/forget/src/CompilerPipeline.ts b/compiler/forget/src/CompilerPipeline.ts index 3ca516125b..fda4f4c7a5 100644 --- a/compiler/forget/src/CompilerPipeline.ts +++ b/compiler/forget/src/CompilerPipeline.ts @@ -23,6 +23,7 @@ import { inferReactiveScopeVariables, propagateScopeDependencies, pruneUnusedLabels, + pruneUnusedLValues, pruneUnusedScopes, renameVariables, } from "./ReactiveScopes"; @@ -82,6 +83,9 @@ export default function ( pruneUnusedScopes(reactiveFunction); logReactiveFunction("pruneUnusedScopes", reactiveFunction); + pruneUnusedLValues(reactiveFunction); + logReactiveFunction("pruneUnusedLValues", reactiveFunction); + renameVariables(reactiveFunction); logReactiveFunction("renameVariables", reactiveFunction); diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 4310f3f21c..3a33048e51 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -649,13 +649,17 @@ function lowerStatement( const stmt = stmtPath as NodePath; const expression = stmt.get("expression"); const value = lowerExpression(builder, expression); - if (expression.isAssignmentExpression()) { - // instruction already emitted via lowerExpression() + if (expression.isAssignmentExpression() && value.kind === "Identifier") { + // already lowered to a place return; } + const place = buildTemporaryPlace( + builder, + stmt.node.loc ?? GeneratedSource + ); builder.push({ id: makeInstructionId(0), - lvalue: null, + lvalue: { kind: InstructionKind.Const, place }, value, loc: stmt.node.loc ?? GeneratedSource, }); @@ -885,13 +889,7 @@ function lowerExpression( // tmp != null ? tmp : const left = lowerExpressionToPlace(builder, leftPath); - const nullPlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: left.loc, - }; + const nullPlace: Place = buildTemporaryPlace(builder, left.loc); builder.push({ id: makeInstructionId(0), value: { @@ -903,13 +901,7 @@ function lowerExpression( lvalue: { place: { ...nullPlace }, kind: InstructionKind.Const }, }); - const condPlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: left.loc, - }; + const condPlace: Place = buildTemporaryPlace(builder, left.loc); builder.push({ id: makeInstructionId(0), lvalue: { @@ -960,36 +952,23 @@ function lowerExpression( } case "MemberExpression": { const leftExpr = left as NodePath; - const object = lowerExpressionToPlace( - builder, - leftExpr.get("object") - ); const property = leftExpr.get("property"); invariant( property.isIdentifier(), "Assignment expression to dynamic properties is not yet supported" ); const right = lowerExpressionToPlace(builder, expr.get("right")); - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, + const object = lowerExpressionToPlace( + builder, + leftExpr.get("object") + ); + return { + kind: "PropertyStore", + object, + property: property.node.name, + value: right, + loc: leftNode.loc ?? GeneratedSource, }; - builder.push({ - id: makeInstructionId(0), - lvalue: { place: { ...place }, kind: InstructionKind.Const }, - value: { - kind: "PropertyStore", - object, - property: property.node.name, - value: right, - loc: leftNode.loc ?? GeneratedSource, - }, - loc: exprLoc, - }); - return place; } default: { todoInvariant( @@ -1056,13 +1035,10 @@ function lowerExpression( "Assignment expression to dynamic properties is not yet supported" ); // Store the previous value to a temporary - const previousValuePlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; + const previousValuePlace: Place = buildTemporaryPlace( + builder, + exprLoc + ); builder.push({ id: makeInstructionId(0), lvalue: { @@ -1078,13 +1054,7 @@ function lowerExpression( loc: leftExpr.node.loc ?? GeneratedSource, }); // Store the new value to a temporary - const newValuePlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; + const newValuePlace: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), lvalue: { @@ -1102,29 +1072,13 @@ function lowerExpression( }); // Save the result back to the property - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; - builder.push({ - id: makeInstructionId(0), - lvalue: { - place: { ...place }, - kind: InstructionKind.Const, - }, - value: { - kind: "PropertyStore", - object: { ...object }, - property: property.node.name, - value: { ...newValuePlace }, - loc: leftExpr.node.loc ?? GeneratedSource, - }, + return { + kind: "PropertyStore", + object: { ...object }, + property: property.node.name, + value: { ...newValuePlace }, loc: leftExpr.node.loc ?? GeneratedSource, - }); - return place; + }; } default: { invariant( @@ -1149,13 +1103,7 @@ function lowerExpression( property: property.node.name, loc: exprLoc, }; - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), lvalue: { place: { ...place }, kind: InstructionKind.Const }, @@ -1231,13 +1179,7 @@ function lowerConditional( consequent: () => InstructionValue, alternate: () => InstructionValue ): Place { - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc, - }; + const place: Place = buildTemporaryPlace(builder, loc); // Block for code following the if const continuationBlock = builder.reserve(); // Block for the consequent (if the test is truthy) @@ -1311,13 +1253,7 @@ function lowerJsxElementName( }; return place; } else { - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: { @@ -1351,13 +1287,7 @@ function lowerJsxElement( todoInvariant(expression.isExpression(), "handle empty expressions"); return lowerExpressionToPlace(builder, expression); } else if (exprPath.isJSXText()) { - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: { @@ -1374,13 +1304,7 @@ function lowerJsxElement( t.isJSXFragment(exprNode) || t.isJSXSpreadChild(exprNode), "Expected refinement to work" ); - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: { @@ -1404,13 +1328,7 @@ function lowerExpressionToPlace( return instr; } const exprLoc = exprPath.node.loc ?? GeneratedSource; - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: instr, @@ -1496,6 +1414,20 @@ function lowerLVal(builder: HIRBuilder, exprPath: NodePath): Place { } } +/** + * Creates a temporary Identifier and Place referencing that identifier. + */ +function buildTemporaryPlace(builder: HIRBuilder, loc: SourceLocation): Place { + const place: Place = { + kind: "Identifier", + identifier: builder.makeTemporary(), + memberPath: null, + effect: Effect.Unknown, + loc, + }; + return place; +} + function lowerAssignment( builder: HIRBuilder, loc: SourceLocation, diff --git a/compiler/forget/src/HIR/Codegen.ts b/compiler/forget/src/HIR/Codegen.ts index 640ec791b8..2f19e26955 100644 --- a/compiler/forget/src/HIR/Codegen.ts +++ b/compiler/forget/src/HIR/Codegen.ts @@ -302,14 +302,7 @@ export function codegenInstruction( if (instr.lvalue === null) { return t.expressionStatement(value); } - if (instr.value.kind === "PropertyStore") { - invariant( - instr.lvalue.place.identifier.name === null, - "Expected property stores to be lowered to a temporary" - ); - temp.set(instr.lvalue.place.identifier.id, value); - return t.expressionStatement(value); - } else if ( + if ( instr.lvalue.place.memberPath === null && instr.lvalue.place.identifier.name === null ) { diff --git a/compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts b/compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts new file mode 100644 index 0000000000..9ce26cf09f --- /dev/null +++ b/compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts @@ -0,0 +1,41 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { + Identifier, + Instruction, + InstructionKind, + ReactiveFunction, +} from "../HIR/HIR"; +import { visitFunction } from "./visitors"; + +/** + * Nulls out lvalues for temporary variables that are never accessed later. This only + * nulls out the lvalue itself, it does not remove the corresponding instructions. + */ +export function pruneTemporaryLValues(fn: ReactiveFunction): void { + const lvalues = new Map(); + visitFunction(fn, { + visitInstruction: (instr) => { + if ( + instr.lvalue !== null && + instr.lvalue.kind === InstructionKind.Const && + instr.lvalue.place.identifier.name === null + ) { + lvalues.set(instr.lvalue.place.identifier, instr); + } + }, + visitValue: (value) => { + if (value.kind === "Identifier") { + lvalues.delete(value.identifier); + } + }, + }); + for (const [, instr] of lvalues) { + instr.lvalue = null; + } +} diff --git a/compiler/forget/src/ReactiveScopes/index.ts b/compiler/forget/src/ReactiveScopes/index.ts index acc1f021f9..2905926ece 100644 --- a/compiler/forget/src/ReactiveScopes/index.ts +++ b/compiler/forget/src/ReactiveScopes/index.ts @@ -12,6 +12,7 @@ export { inferReactiveScopes } from "./InferReactiveScopes"; export { inferReactiveScopeVariables } from "./InferReactiveScopeVariables"; export { printReactiveFunction } from "./PrintReactiveFunction"; export { propagateScopeDependencies } from "./PropagateScopeDependencies"; +export { pruneTemporaryLValues as pruneUnusedLValues } from "./PruneTemporaryLValues"; export { pruneUnusedLabels } from "./PruneUnusedLabels"; export { pruneUnusedScopes } from "./PruneUnusedScopes"; export { renameVariables } from "./RenameVariables"; diff --git a/compiler/forget/src/ReactiveScopes/visitors.ts b/compiler/forget/src/ReactiveScopes/visitors.ts index c46c3a7c7d..75c34f1b9e 100644 --- a/compiler/forget/src/ReactiveScopes/visitors.ts +++ b/compiler/forget/src/ReactiveScopes/visitors.ts @@ -5,9 +5,82 @@ * LICENSE file in the root directory of this source tree. */ -import { ReactiveBlock, ReactiveTerminal } from "../HIR/HIR"; +import { + Instruction, + InstructionValue, + Place, + ReactiveBlock, + ReactiveFunction, + ReactiveScope, + ReactiveTerminal, + ReactiveValueBlock, +} from "../HIR/HIR"; +import { eachInstructionValueOperand } from "../HIR/visitors"; import { assertExhaustive } from "../Utils/utils"; +export function visitFunction( + fn: ReactiveFunction, + visitors: { + visitValue?: (value: InstructionValue) => void; + visitInstruction?: (instr: Instruction) => void; + visitTerminal?: (terminal: ReactiveTerminal) => void; + visitScope?: (scope: ReactiveScope) => void; + } +): void { + const { visitValue, visitInstruction, visitTerminal, visitScope } = visitors; + function visitBlock(block: ReactiveBlock): void { + for (const item of block) { + switch (item.kind) { + case "instruction": { + if (visitValue) { + for (const operand of eachInstructionValueOperand( + item.instruction.value + )) { + visitValue(operand); + } + } + if (visitInstruction) { + visitInstruction(item.instruction); + } + break; + } + case "terminal": { + if (visitValue) { + eachTerminalOperand(item.terminal, (operand) => { + visitValue(operand); + }); + } + if (visitTerminal) { + visitTerminal(item.terminal); + } + eachTerminalBlock(item.terminal, visitBlock, visitValueBlock); + break; + } + case "scope": { + if (visitScope) { + visitScope(item.scope); + } + visitBlock(item.instructions); + break; + } + default: { + assertExhaustive( + item, + `Unexpected item kind '${(item as any).kind}'` + ); + } + } + } + } + function visitValueBlock(block: ReactiveValueBlock): void { + visitBlock(block.instructions); + if (block.value !== null && visitValue) { + visitValue(block.value); + } + } + visitBlock(fn.body); +} + export function mapTerminalBlocks( terminal: ReactiveTerminal, fn: (block: ReactiveBlock) => ReactiveBlock @@ -50,3 +123,98 @@ export function mapTerminalBlocks( } } } + +export function eachTerminalBlock( + terminal: ReactiveTerminal, + visitBlock: (block: ReactiveBlock) => void, + visitValueBlock: (block: ReactiveValueBlock) => void +): void { + switch (terminal.kind) { + case "break": + case "continue": + case "return": + case "throw": { + break; + } + case "for": { + visitValueBlock(terminal.init); + visitValueBlock(terminal.test); + visitValueBlock(terminal.update); + visitBlock(terminal.loop); + break; + } + case "while": { + visitValueBlock(terminal.test); + visitBlock(terminal.loop); + break; + } + case "if": { + visitBlock(terminal.consequent); + if (terminal.alternate !== null) { + visitBlock(terminal.alternate); + } + break; + } + case "switch": { + for (const case_ of terminal.cases) { + if (case_.block !== undefined) { + visitBlock(case_.block); + } + } + break; + } + default: { + assertExhaustive( + terminal, + `Unexpected terminal kind '${(terminal as any).kind}'` + ); + } + } +} + +export function eachTerminalOperand( + terminal: ReactiveTerminal, + fn: (place: Place) => void +): void { + switch (terminal.kind) { + case "break": + case "continue": { + break; + } + case "return": { + if (terminal.value !== null) { + fn(terminal.value); + } + break; + } + case "throw": { + fn(terminal.value); + break; + } + case "for": { + break; + } + case "while": { + break; + } + case "if": { + fn(terminal.test); + break; + } + case "switch": { + fn(terminal.test); + for (const case_ of terminal.cases) { + if (case_.test !== null) { + fn(case_.test); + } + } + break; + } + default: { + assertExhaustive( + terminal, + `Unexpected terminal kind '${(terminal as any).kind}'` + ); + } + } +} diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md b/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md deleted file mode 100644 index 9bdf873991..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md +++ /dev/null @@ -1,28 +0,0 @@ - -## Input - -```javascript -function foo() { - const x = { y: 0 }; - const y = { z: 0 }; - x.y += y.z *= 1; -} - -``` - -## Code - -```javascript -function foo() { - const x = { - y: 0, - }; - const y = { - z: 0, - }; - y.z = y.z * 1; - x.y = x.y + (y.z = y.z * 1); -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js b/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js deleted file mode 100644 index b6e035f072..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js +++ /dev/null @@ -1,5 +0,0 @@ -function foo() { - const x = { y: 0 }; - const y = { z: 0 }; - x.y += y.z *= 1; -} diff --git a/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md new file mode 100644 index 0000000000..8629728d77 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md @@ -0,0 +1,35 @@ + +## Input + +```javascript +function foo() { + const x = { x: 0 }; + const y = { z: 0 }; + const z = { z: 0 }; + x.x += y.y *= 1; + z.z += y.y *= x.x &= 3; + return z; +} + +``` + +## Code + +```javascript +function foo() { + const x = { + x: 0, + }; + const y = { + z: 0, + }; + const z = { + z: 0, + }; + x.x = x.x + (y.y = y.y * 1); + z.z = z.z + (y.y = y.y * (x.x = x.x & 3)); + return z; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js new file mode 100644 index 0000000000..fb4f27844c --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js @@ -0,0 +1,8 @@ +function foo() { + const x = { x: 0 }; + const y = { z: 0 }; + const z = { z: 0 }; + x.x += y.y *= 1; + z.z += y.y *= x.x &= 3; + return z; +}