From 73ad571c6b52cd7e5321ab52c81c5a133fd1d201 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 14 Dec 2022 08:42:35 -0800 Subject: [PATCH] Add invariant for problematic LeaveSSA case The new LeaveSSA looks ahead to the phis of fallback blocks. However, HIR can sometimes have multiple blocks with the same fallthrough (totally fine), so this diff clears the phis of fallbacks as they are reached to avoid reprocessing them. This caused a previously incorrect case to now fail, yay. --- compiler/forget/src/HIR/HIRBuilder.ts | 18 +++-- compiler/forget/src/HIR/LeaveSSA.ts | 15 +++- compiler/forget/src/HIR/Pipeline.ts | 27 +++---- compiler/forget/src/HIR/logger.ts | 11 +++ .../hir/_bug_inverted-if-else.expect.md | 72 ------------------- .../hir/error.inverted-if-else.expect.md | 26 +++++++ ...d-if-else.js => error.inverted-if-else.js} | 0 7 files changed, 72 insertions(+), 97 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/hir/_bug_inverted-if-else.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/error.inverted-if-else.expect.md rename compiler/forget/src/__tests__/fixtures/hir/{_bug_inverted-if-else.js => error.inverted-if-else.js} (100%) diff --git a/compiler/forget/src/HIR/HIRBuilder.ts b/compiler/forget/src/HIR/HIRBuilder.ts index ee0be73948..9c6dac2dcd 100644 --- a/compiler/forget/src/HIR/HIRBuilder.ts +++ b/compiler/forget/src/HIR/HIRBuilder.ts @@ -23,6 +23,7 @@ import { makeType, Terminal, } from "./HIR"; +import { logHIR } from "./logger"; import { printInstruction } from "./PrintHIR"; import { eachTerminalSuccessor, mapTerminalSuccessors } from "./visitors"; @@ -156,16 +157,19 @@ export default class HIRBuilder { preds: new Set(), phis: new Set(), }); - // First reduce indirections and prune unreachable blocks - let reduced = shrink({ + let ir: HIR = { blocks: this.#completed, entry: this.#entry, - }); + }; + logHIR("Build (pre-shrink)", ir); + // First reduce indirections and prune unreachable blocks + let shrunk = shrink(ir); + logHIR("Build (shrunk)", shrunk); // then convert to reverse postorder - const blocks = reversePostorderBlocks(reduced); - markInstructionIds(blocks); - markPredecessors(blocks); - return blocks; + const rpo = reversePostorderBlocks(shrunk); + markInstructionIds(rpo); + markPredecessors(rpo); + return rpo; } /** diff --git a/compiler/forget/src/HIR/LeaveSSA.ts b/compiler/forget/src/HIR/LeaveSSA.ts index 99cfda66fa..8579a63a47 100644 --- a/compiler/forget/src/HIR/LeaveSSA.ts +++ b/compiler/forget/src/HIR/LeaveSSA.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import invariant from "invariant"; import { Effect, GeneratedSource, @@ -34,6 +35,11 @@ export function leaveSSA(fn: HIRFunction) { const hasDeclaration: Set = new Set(); for (const [, block] of fn.body.blocks) { + invariant( + block.phis.size === 0, + "Expected all phis to be cleared by predecessors" + ); + // Identifiers (from phis) that *may* need a new `let` declaration created. If the original // variable declaration flows into the phi, then we can reuse its declaration - this is // discovered during iteration of instructions. @@ -53,18 +59,25 @@ export function leaveSSA(fn: HIRFunction) { ) { const fallthrough = fn.body.blocks.get(terminal.fallthrough)!; phis.push(...fallthrough.phis); + fallthrough.phis.clear(); } if (terminal.kind === "while" || terminal.kind === "for") { const test = fn.body.blocks.get(terminal.test)!; phis.push(...test.phis); + test.phis.clear(); + const loop = fn.body.blocks.get(terminal.loop)!; phis.push(...loop.phis); + loop.phis.clear(); } if (terminal.kind === "for") { const init = fn.body.blocks.get(terminal.init)!; phis.push(...init.phis); + init.phis.clear(); + const update = fn.body.blocks.get(terminal.update)!; phis.push(...update.phis); + update.phis.clear(); // find declarations in the for init for (const instr of init.instructions) { @@ -174,8 +187,6 @@ export function leaveSSA(fn: HIRFunction) { }; block.instructions.push(instr); } - - block.phis.clear(); } } diff --git a/compiler/forget/src/HIR/Pipeline.ts b/compiler/forget/src/HIR/Pipeline.ts index 7eecfcf242..8de4122349 100644 --- a/compiler/forget/src/HIR/Pipeline.ts +++ b/compiler/forget/src/HIR/Pipeline.ts @@ -18,9 +18,8 @@ import { inferMutableRanges } from "./InferMutableRanges"; import { inferReactiveScopeDependencies } from "./InferReactiveScopeDependencies"; import { inferReactiveScopes } from "./InferReactiveScopes"; import { inferReactiveScopeVariables } from "./InferReactiveScopeVariables"; -import { log } from "./logger"; -import { printFunction } from "./PrintHIR"; import { inferTypes } from "./InferTypes"; +import { logHIRFunction } from "./logger"; export type CompilerFlags = { eliminateRedundantPhi: boolean; @@ -46,47 +45,47 @@ export default function ( const env = new Environment(); const ir = lower(func, env); - logStep("HIR", ir); + logHIRFunction("HIR", ir); enterSSA(ir, env); - logStep("SSA", ir); + logHIRFunction("SSA", ir); if (flags.eliminateRedundantPhi) { eliminateRedundantPhi(ir); - logStep("eliminateRedundantPhi", ir); + logHIRFunction("eliminateRedundantPhi", ir); } if (flags.inferTypes) { inferTypes(ir); - logStep("inferTypes", ir); + logHIRFunction("inferTypes", ir); } if (flags.inferReferenceEffects) { inferReferenceEffects(ir); - logStep("inferReferenceEffects", ir); + logHIRFunction("inferReferenceEffects", ir); } if (flags.inferMutableRanges) { inferMutableRanges(ir); - logStep("inferMutableRanges", ir); + logHIRFunction("inferMutableRanges", ir); } if (flags.leaveSSA) { leaveSSA(ir); - logStep("leaveSSA", ir); + logHIRFunction("leaveSSA", ir); } if (flags.inferReactiveScopeVariables) { inferReactiveScopeVariables(ir); - logStep("inferReactiveScopeVariables", ir); + logHIRFunction("inferReactiveScopeVariables", ir); } if (flags.inferReactiveScopes) { inferReactiveScopes(ir); - logStep("inferReactiveScopes", ir); + logHIRFunction("inferReactiveScopes", ir); } if (flags.inferReactiveScopeDependencies) { inferReactiveScopeDependencies(ir); - logStep("inferReactiveScopeDependencies", ir); + logHIRFunction("inferReactiveScopeDependencies", ir); } if (flags.codegen) { @@ -98,7 +97,3 @@ export default function ( return { ast: null, ir: ir }; } - -function logStep(step: string, ir: HIRFunction) { - log(() => `${step}:\n${printFunction(ir)}`); -} diff --git a/compiler/forget/src/HIR/logger.ts b/compiler/forget/src/HIR/logger.ts index 7beaa4e614..368dd729ea 100644 --- a/compiler/forget/src/HIR/logger.ts +++ b/compiler/forget/src/HIR/logger.ts @@ -5,12 +5,23 @@ * LICENSE file in the root directory of this source tree. */ +import { HIR, HIRFunction } from "./HIR"; +import printHIR, { printFunction } from "./PrintHIR"; + let ENABLED: boolean = false; export function toggleLogging(enabled: boolean) { ENABLED = enabled; } +export function logHIR(step: string, ir: HIR): void { + log(() => `${step}:\n${printHIR(ir)}`); +} + +export function logHIRFunction(step: string, fn: HIRFunction): void { + log(() => `${step}:\n${printFunction(fn)}`); +} + export function log(fn: () => string) { if (ENABLED) { const message = fn(); diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_inverted-if-else.expect.md b/compiler/forget/src/__tests__/fixtures/hir/_bug_inverted-if-else.expect.md deleted file mode 100644 index be79931a22..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_inverted-if-else.expect.md +++ /dev/null @@ -1,72 +0,0 @@ - -## Input - -```javascript -function foo(a, b, c) { - let x = null; - label: { - if (a) { - x = b; - break label; - } - x = c; - } - return x; -} - -``` - -## HIR - -``` -bb0: - [1] Const mutate x$8_@0:TPrimitive = null - [2] If (read a$5) then:bb3 else:bb2 fallthrough=bb2 -bb3: - predecessor blocks: bb0 - [3] Const mutate x$9_@1 = read b$6 - [4] Goto bb1 -bb2: - predecessor blocks: bb0 - [5] Const mutate x$10_@2 = read c$7 - [6] Goto bb1 -bb1: - predecessor blocks: bb3 bb2 - [7] Return read x$11 -scope1 [3:4]: - - dependency: read b$6 -scope2 [5:6]: - - dependency: read c$7 -``` - -## Reactive Scopes - -``` -function foo( - a, - b, - c, -) { - [1] Const mutate x$8_@0:TPrimitive = null - if (read a$5) { - [3] Const mutate x$9_@1 = read b$6 - } - [5] Const mutate x$10_@2 = read c$7 -} - -``` - -## Code - -```javascript -function foo$0(a$5, b$6, c$7) { - const x$8 = null; - bb2: if (a$5) { - const x$9 = b$6; - } - - const x$10 = c$7; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/error.inverted-if-else.expect.md b/compiler/forget/src/__tests__/fixtures/hir/error.inverted-if-else.expect.md new file mode 100644 index 0000000000..4c0bb21f3a --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/error.inverted-if-else.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +function foo(a, b, c) { + let x = null; + label: { + if (a) { + x = b; + break label; + } + x = c; + } + return x; +} + +``` + + +## Error + +``` +Expected all phis to be cleared by predecessors +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_inverted-if-else.js b/compiler/forget/src/__tests__/fixtures/hir/error.inverted-if-else.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/hir/_bug_inverted-if-else.js rename to compiler/forget/src/__tests__/fixtures/hir/error.inverted-if-else.js