diff --git a/packages/bugc/CHANGELOG.md b/packages/bugc/CHANGELOG.md index 1b265637af..edf0b1d97b 100644 --- a/packages/bugc/CHANGELOG.md +++ b/packages/bugc/CHANGELOG.md @@ -52,6 +52,10 @@ support. Changes to the specification itself are tracked in the root - `push` of bytes in calldata onto an `array` in storage now stores the bytes. Before, it stored the calldata reference's word (offset and length) ([#379]). +- A local defined before a call keeps its pointer after the call. + The dominator tree that local-variable tracking uses left out a + call's edge to its continuation, so every local defined before a call + was listed with no pointer from the call on ([#378]). - Reading a `string` or `bytes` from storage as a whole, as in `let m: string = motd;`, now copies it to memory, decoded from its storage encoding. This also corrects an index (`data[3]`), a slice @@ -525,4 +529,5 @@ First publication. [#370]: https://github.com/ethdebug/format/pull/370 [#371]: https://github.com/ethdebug/format/pull/371 [#377]: https://github.com/ethdebug/format/pull/377 +[#378]: https://github.com/ethdebug/format/pull/378 [#379]: https://github.com/ethdebug/format/pull/379 diff --git a/packages/bugc/src/evmgen/debug/local-variables.soundness.test.ts b/packages/bugc/src/evmgen/debug/local-variables.soundness.test.ts index 90ce23d395..554f90ec32 100644 --- a/packages/bugc/src/evmgen/debug/local-variables.soundness.test.ts +++ b/packages/bugc/src/evmgen/debug/local-variables.soundness.test.ts @@ -39,6 +39,24 @@ function codeOffset(context: unknown): number | undefined { ?.code?.range?.offset; } +/** A struct local, a call, then the local again */ +const acrossCall = `name AcrossCall; +define { + struct P { a: uint64; b: uint32; }; + function f() -> uint256 { return r + 3; }; +} +storage { [0] r: uint256; [1] ps: mapping; } +create {} +code { + let p: P = ps[msg.sender]; + p.a = p.a + 5; + let h = f(); + if (h == 0) { return; } + p.b = p.b + 1; + ps[msg.sender] = p; + r = h; +}`; + describe("which locals are listed", () => { it("(a) does not list a local before its declaration", async () => { const source = `name PreDef; @@ -157,6 +175,22 @@ code { expect(rows.some(({ names }) => names.includes("i"))).toBe(true); }); + it("(g) keeps a local located after a call", async () => { + // A call's continuation is dominated by the calling block: wherever + // the call's result `h` is located, so is `p`, defined before it + for (const level of levels) { + const { program } = await traceLocals(acrossCall, level); + const located = program.instructions + .map((instruction) => localsOf(instruction.context)) + .filter((vs) => vs.some((v) => v.identifier === "h" && v.pointer)); + expect(located.length, `O${level}`).toBeGreaterThan(0); + for (const vs of located) { + const p = vs.find((v) => v.identifier === "p"); + expect(p?.pointer, `O${level}`).toBeDefined(); + } + } + }); + it("(f) gives a stack-resident local a stack pointer", async () => { const { program } = await traceLocals(programs[0].source); const pointers = program.instructions.flatMap((i) => @@ -852,6 +886,28 @@ code { k: { values: [7n] }, }, }, + { + name: "a struct local across a call", + source: acrossCall, + locals: { + p: { + shape: { + kind: "struct", + fields: { + a: { kind: "scalar", size: 8 }, + b: { kind: "scalar", size: 4 }, + }, + }, + everyLevel: true, + values: [ + { a: 0n, b: 0n }, + { value: { a: 5n, b: 0n }, after: "p.a = p.a + 5" }, + { value: { a: 5n, b: 1n }, after: "p.b = p.b + 1" }, + ], + }, + h: { values: [3n] }, + }, + }, { name: "a void call", source: `name VoidCall; diff --git a/packages/bugc/src/ir/analysis/stats.test.ts b/packages/bugc/src/ir/analysis/stats.test.ts index 4c13dc7eb4..624fe288b9 100644 --- a/packages/bugc/src/ir/analysis/stats.test.ts +++ b/packages/bugc/src/ir/analysis/stats.test.ts @@ -173,6 +173,52 @@ describe("Statistics", () => { }); }); + describe("calls", () => { + const source = ` + name Test; + define { + function f(n: uint256) -> uint256 { return n + 1; }; + } + storage { [0] x: uint256; } + code { + for (let i = 0; i < 3; i = i + 1) { + let y = f(i); + x = x + y; + } + return; + } + `; + + it("follows a call to its continuation", async () => { + const ir = await compileToIr(source); + const call = [...ir.main.blocks.entries()].find( + ([, block]) => block.terminator.kind === "call", + )!; + const terminator = call[1].terminator; + if (terminator.kind !== "call") throw new Error("no call"); + const { continuation } = terminator; + + const dom = new Statistics.Analyzer().analyze(ir).dominatorTree; + expect(dom[continuation]).toBe(call[0]); + + // without call edges, no block after a call has a dominator + const without = new Statistics.Analyzer({ callEdges: false }).analyze( + ir, + ).dominatorTree; + expect(without[continuation]).toBeUndefined(); + }); + + it("finds a loop's blocks through a call", async () => { + const ir = await compileToIr(source); + const { loopInfo } = new Statistics.Analyzer().analyze(ir); + expect(loopInfo).toHaveLength(1); + const body = [...ir.main.blocks.keys()].find((id) => + id.startsWith("for_body"), + )!; + expect(loopInfo[0].blocks).toContain(body); + }); + }); + describe("detectLoops", () => { it("should detect nested loops", async () => { const source = ` diff --git a/packages/bugc/src/ir/analysis/stats.ts b/packages/bugc/src/ir/analysis/stats.ts index af4ee8dbc4..d4459b98dc 100644 --- a/packages/bugc/src/ir/analysis/stats.ts +++ b/packages/bugc/src/ir/analysis/stats.ts @@ -32,7 +32,23 @@ export namespace Statistics { depth: number; } + export interface Options { + /** + * Whether a call's continuation is a successor of the calling + * block (default true). Without call edges, no block after a call + * has a dominator: common-subexpression elimination relies on this + * to reuse no value across a call. + */ + callEdges?: boolean; + } + export class Analyzer { + private readonly callEdges: boolean; + + constructor({ callEdges = true }: Options = {}) { + this.callEdges = callEdges; + } + analyze(module: Ir.Module): Statistics { const func = module.main; const blocks = Array.from(func.blocks.values()); @@ -160,6 +176,9 @@ export namespace Statistics { case "branch": edges += 2; break; + case "call": + if (this.callEdges) edges += 1; + break; // return has no edges } } @@ -174,13 +193,7 @@ export namespace Statistics { const dominators: Record = {}; const blockIds = Array.from(func.blocks.keys()); - // Build predecessor map for efficiency - const predecessors: Record = {}; - for (const blockId of blockIds) { - predecessors[blockId] = Array.from( - func.blocks.get(blockId)?.predecessors || [], - ); - } + const predecessors = this.predecessorsOf(func); // Entry block dominates itself (has no dominator) dominators[func.entry] = null; @@ -395,6 +408,7 @@ export namespace Statistics { func: Ir.Function, ): string[] { // Find all blocks in the loop (simplified) + const predecessors = this.predecessorsOf(func); const loopBlocks = new Set([header, tail]); const worklist = [tail]; @@ -403,7 +417,7 @@ export namespace Statistics { const block = func.blocks.get(blockId); if (!block) continue; - for (const pred of block.predecessors) { + for (const pred of predecessors[blockId] ?? []) { if (!loopBlocks.has(pred)) { loopBlocks.add(pred); if (pred !== header) { @@ -416,12 +430,37 @@ export namespace Statistics { return Array.from(loopBlocks); } + /** + * Each block's predecessors. With call edges, from the terminators: + * `Block.predecessors` omits a call's edge to its continuation. + * Without, `Block.predecessors` as it is. + */ + private predecessorsOf(func: Ir.Function): Record { + const predecessors: Record = {}; + for (const [blockId, block] of func.blocks) { + predecessors[blockId] = this.callEdges + ? [] + : Array.from(block.predecessors || []); + } + if (!this.callEdges) return predecessors; + for (const [blockId, block] of func.blocks) { + for (const succ of this.getSuccessors(block)) { + // (a successor that is not one of the function's blocks has + // no entry, and is skipped) + predecessors[succ]?.push(blockId); + } + } + return predecessors; + } + private getSuccessors(block: Ir.Block): string[] { switch (block.terminator.kind) { case "jump": return [block.terminator.target]; case "branch": return [block.terminator.trueTarget, block.terminator.falseTarget]; + case "call": + return this.callEdges ? [block.terminator.continuation] : []; case "return": return []; default: diff --git a/packages/bugc/src/irgen/generate/function.ts b/packages/bugc/src/irgen/generate/function.ts index 01d7512d08..84f3edf988 100644 --- a/packages/bugc/src/irgen/generate/function.ts +++ b/packages/bugc/src/irgen/generate/function.ts @@ -135,7 +135,12 @@ function computePredecessors( } break; } - // "return" and "unreachable" have no successors + // "return" and "unreachable" have no successors. A "call"'s edge + // to its continuation is left out: the passes that read + // `predecessors` (block merging, jump optimization, return + // merging, tail calls) see the CFG without it, and adding it here + // can change the code they make. Dominators and loops + // (Statistics.Analyzer) derive their own, with call edges. } } diff --git a/packages/bugc/src/optimizer/steps/common-subexpression-elimination.test.ts b/packages/bugc/src/optimizer/steps/common-subexpression-elimination.test.ts new file mode 100644 index 0000000000..564e1970f8 --- /dev/null +++ b/packages/bugc/src/optimizer/steps/common-subexpression-elimination.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect } from "vitest"; + +import { compile } from "#compiler"; +import type * as Ir from "#ir"; + +describe("CommonSubexpressionEliminationStep", () => { + // `msg.value` before and after a call that is not inlined (it + // recurses): computing it again is cheaper than keeping it live + // across the call, so the value is not reused + const source = `name Rec; +define { + function g(n: uint256) -> uint256 { + let a = msg.value + n; + if (n == 0) { return a; } + let b = g(n - 1); + return a + b + msg.value; + }; +} +storage { [0] r: uint256; } +create {} +code { r = g(3); }`; + + for (const level of [2, 3] as const) { + it(`reuses no value across a call (level ${level})`, async () => { + const result = await compile({ + to: "ir", + source, + optimizer: { level }, + }); + if (!result.success) throw new Error("compile failed"); + const g = result.value.ir.functions.get("g")!; + const values = [...g.blocks.values()] + .flatMap((block) => block.instructions) + .filter( + (inst): inst is Ir.Instruction.Env => + inst.kind === "env" && inst.op === "msg_value", + ); + expect(values).toHaveLength(2); + }); + } +}); diff --git a/packages/bugc/src/optimizer/steps/common-subexpression-elimination.ts b/packages/bugc/src/optimizer/steps/common-subexpression-elimination.ts index a9f07ff32f..84c0f27878 100644 --- a/packages/bugc/src/optimizer/steps/common-subexpression-elimination.ts +++ b/packages/bugc/src/optimizer/steps/common-subexpression-elimination.ts @@ -13,8 +13,12 @@ export class CommonSubexpressionEliminationStep extends BaseOptimizationStep { // Process each function separately this.processAllFunctions(optimized, (func) => { - // Compute dominator tree for this function - const analyzer = new Ir.Analysis.Statistics.Analyzer(); + // Compute dominator tree for this function, without call edges: + // no value is reused across a call (keeping one live across a + // call can cost more than computing it again) + const analyzer = new Ir.Analysis.Statistics.Analyzer({ + callEdges: false, + }); const analysis = analyzer.analyze({ ...module, main: func }); const dominators = analysis.dominatorTree;