From fe2d179a619012ae1cea44f3efd97c9974a6d5d5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 3 Mar 2023 17:09:26 -0800 Subject: [PATCH] Value block reassignment uses StoreLocal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As part of removing Instruction.lvalue we need to ensure that it is only used to represent that instruction's value — the InstructionKind should always be Const. The one place where we violated this was for value blocks, specifically ConditionalExpression and LogicalExpression. For both of those, we generate a single temporary place to represent the expression result. Then the consequent and alternate branch ended in a `LoadLocal` that reassigned that temporary (in the lvalue) to the result of that branch. This PR changes to use StoreLocal instead, and updates the recently added validation pass to ensure that all identifiers that appear in an Instruction.lvalue are only ever assigned once. --- compiler/forget/src/CompilerPipeline.ts | 2 + compiler/forget/src/HIR/BuildHIR.ts | 43 +++++++++--- .../src/HIR/ValidateConsistentIdentifiers.ts | 11 +++ .../ReactiveScopes/BuildReactiveFunction.ts | 69 ++++++++++++++++--- 4 files changed, 106 insertions(+), 19 deletions(-) diff --git a/compiler/forget/src/CompilerPipeline.ts b/compiler/forget/src/CompilerPipeline.ts index 50ed36278f..cae551ad8e 100644 --- a/compiler/forget/src/CompilerPipeline.ts +++ b/compiler/forget/src/CompilerPipeline.ts @@ -58,6 +58,8 @@ export function* run( mergeConsecutiveBlocks(hir); yield log({ kind: "hir", name: "MergeConsecutiveBlocks", value: hir }); + validateConsistentIdentifiers(hir); + enterSSA(hir); yield log({ kind: "hir", name: "SSA", value: hir }); diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 6d3ddb61c0..c7e7827837 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -1041,10 +1041,19 @@ function lowerExpression( // Block for the consequent (if the test is truthy) const consequentBlock = builder.enter("value", (_blockId) => { + const consequent = lowerExpressionToTemporary( + builder, + expr.get("consequent") + ); builder.push({ id: makeInstructionId(0), - lvalue: { ...place }, - value: lowerExpression(builder, expr.get("consequent")), + lvalue: buildTemporaryPlace(builder, exprLoc), + value: { + kind: "StoreLocal", + lvalue: { kind: InstructionKind.Const, place: { ...place } }, + value: consequent, + loc: exprLoc, + }, loc: exprLoc, }); return { @@ -1056,10 +1065,19 @@ function lowerExpression( }); // Block for the alternate (if the test is not truthy) const alternateBlock = builder.enter("value", (_blockId) => { + const alternate = lowerExpressionToTemporary( + builder, + expr.get("alternate") + ); builder.push({ id: makeInstructionId(0), - lvalue: { ...place }, - value: lowerExpression(builder, expr.get("alternate")), + lvalue: buildTemporaryPlace(builder, exprLoc), + value: { + kind: "StoreLocal", + lvalue: { kind: InstructionKind.Const, place: { ...place } }, + value: alternate, + loc: exprLoc, + }, loc: exprLoc, }); return { @@ -1106,10 +1124,11 @@ function lowerExpression( const consequent = builder.enter("value", () => { builder.push({ id: makeInstructionId(0), - lvalue: { ...place }, + lvalue: buildTemporaryPlace(builder, leftPlace.loc), value: { - kind: "LoadLocal", - place: { ...leftPlace }, + kind: "StoreLocal", + lvalue: { kind: InstructionKind.Const, place: { ...place } }, + value: { ...leftPlace }, loc: leftPlace.loc, }, loc: exprLoc, @@ -1122,10 +1141,16 @@ function lowerExpression( }; }); const alternate = builder.enter("value", () => { + const right = lowerExpressionToTemporary(builder, expr.get("right")); builder.push({ id: makeInstructionId(0), - lvalue: { ...place }, - value: lowerExpression(builder, expr.get("right")), + lvalue: buildTemporaryPlace(builder, right.loc), + value: { + kind: "StoreLocal", + lvalue: { kind: InstructionKind.Const, place: { ...place } }, + value: { ...right }, + loc: right.loc, + }, loc: exprLoc, }); return { diff --git a/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts b/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts index 52bff620af..6444ea7271 100644 --- a/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts +++ b/compiler/forget/src/HIR/ValidateConsistentIdentifiers.ts @@ -13,6 +13,7 @@ import { IdentifierId, SourceLocation, } from "./HIR"; +import { printPlace } from "./PrintHIR"; import { eachInstructionValueOperand, eachTerminalOperand } from "./visitors"; /** @@ -21,6 +22,7 @@ import { eachInstructionValueOperand, eachTerminalOperand } from "./visitors"; */ export function validateConsistentIdentifiers(fn: HIRFunction): void { const identifiers: Identifiers = new Map(); + const assignments: Set = new Set(); for (const [, block] of fn.body.blocks) { for (const phi of block.phis) { validate(identifiers, phi.id); @@ -35,6 +37,15 @@ export function validateConsistentIdentifiers(fn: HIRFunction): void { instr.lvalue.loc ); } + if (assignments.has(instr.lvalue.identifier.id)) { + CompilerError.invariant( + `Expected lvalues to be assigned exactly once, found duplicate assignment of '${printPlace( + instr.lvalue + )}'`, + instr.lvalue.loc + ); + } + assignments.add(instr.lvalue.identifier.id); validate(identifiers, instr.lvalue.identifier, instr.lvalue.loc); for (const operand of eachInstructionValueOperand(instr.value)) { validate(identifiers, operand.identifier, operand.loc); diff --git a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts index 4dc5f48356..ca8b0757c0 100644 --- a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -519,16 +519,9 @@ class Driver { loc: SourceLocation ): { block: BlockId; value: ReactiveValue; place: Place; id: InstructionId } { const defaultBlock = this.cx.ir.blocks.get(id)!; - if ( - defaultBlock.terminal.kind === "goto" || - defaultBlock.terminal.kind === "branch" - ) { + if (defaultBlock.terminal.kind === "branch") { const instructions = defaultBlock.instructions; if (instructions.length === 0) { - invariant( - defaultBlock.terminal.kind === "branch", - "Expected instructions for non-branch terminal" - ); return { block: defaultBlock.id, place: defaultBlock.terminal.test, @@ -541,6 +534,11 @@ class Driver { }; } else if (defaultBlock.instructions.length === 1) { const instr = defaultBlock.instructions[0]!; + invariant( + instr.lvalue.identifier.id === + defaultBlock.terminal.test.identifier.id, + "Expected branch block to end in an instruction that sets the test value" + ); return { block: defaultBlock.id, place: instr.lvalue!, @@ -558,7 +556,58 @@ class Driver { }; return { block: defaultBlock.id, - place: instr.lvalue!, + place: defaultBlock.terminal.test, + value: sequence, + id: defaultBlock.terminal.id, + }; + } + } else if (defaultBlock.terminal.kind === "goto") { + const instructions = defaultBlock.instructions; + if (instructions.length === 0) { + invariant( + false, + "Expected goto value block to have at least one instruction" + ); + } else if (defaultBlock.instructions.length === 1) { + const instr = defaultBlock.instructions[0]!; + let place: Place = instr.lvalue!; + let value: ReactiveValue = instr.value; + if (instr.value.kind === "StoreLocal") { + place = instr.value.lvalue.place; + value = { + kind: "LoadLocal", + place: instr.value.value, + loc: instr.value.value.loc, + }; + } + return { + block: defaultBlock.id, + place, + value, + id: instr.id, + }; + } else { + const instr = defaultBlock.instructions.at(-1)!; + let place: Place = instr.lvalue!; + let value: ReactiveValue = instr.value; + if (instr.value.kind === "StoreLocal") { + place = instr.value.lvalue.place; + value = { + kind: "LoadLocal", + place: instr.value.value, + loc: instr.value.value.loc, + }; + } + const sequence: ReactiveSequenceValue = { + kind: "SequenceExpression", + instructions: defaultBlock.instructions.slice(0, -1), + id: instr.id, + value, + loc: loc, + }; + return { + block: defaultBlock.id, + place, value: sequence, id: instr.id, }; @@ -675,7 +724,7 @@ class Driver { }; invariant( consequent.place.identifier === alternate.place.identifier, - "Expected the consquent and alternate of a ternary to store a value to the same place" + "Expected the consequent and alternate of a ternary to store a value to the same place" ); return { place: { ...consequent.place },