bugc: fix -O2/-O3 codegen for call arguments and continuations

packages/bugc/CHANGELOG.md

@@ -56,6 +56,15 @@ support. Changes to the specification itself are tracked in the root
 
 ### Fixed
 
+- At optimization level 2 and up, a call's argument may repeat an
+  earlier computation (as `f(msg.sender)` after a use of
+  `msg.sender`). Before, common subexpression elimination removed the
+  repeat but left the call using it, and code generation failed
+  ("Cannot load value") ([#388]).
+- At optimization level 3, a call followed by `return;` compiles when
+  the function has another `return;`. Before, return merging removed
+  the call's continuation block, and code generation failed ("Jump
+  target ... not found") ([#388]).
 - `push` of bytes in calldata onto an `array<bytes>` in storage now
   stores the bytes. Before, it stored the calldata reference's word
   (offset and length) ([#379]).
@@ -553,3 +562,4 @@ First publication.
 [#379]: https://github.com/ethdebug/format/pull/379
 [#383]: https://github.com/ethdebug/format/pull/383
 [#384]: https://github.com/ethdebug/format/pull/384
+[#388]: https://github.com/ethdebug/format/pull/388

packages/bugc/src/evmgen/__snapshots__/activation-pairing.test.ts.snap

@@ -48,8 +48,8 @@ exports[`the code does not depend on the markers > is the same for every example
   "advanced/nested-structs.bug at O3": "d141d683602322f4195283df977a6c5618e72e995bfca7d8b285ae3a0f779bb4",
   "advanced/token-registry.bug at O0": "430a947f02160f905a04efdeea0b5d69fb7aa6be6b2dc54734be26ec0511988a",
   "advanced/token-registry.bug at O1": "2de1cfe1d642a35a409d46df00f9ebb08ce0c43736020a5aa06cae30983b72e6",
-  "advanced/token-registry.bug at O2": "does not compile",
-  "advanced/token-registry.bug at O3": "does not compile",
+  "advanced/token-registry.bug at O2": "2c0dcbce9d26b8182ce6607aeb480005fc485e7e21c44405be48c451c945c2e3",
+  "advanced/token-registry.bug at O3": "989c5871f2e3c40b13af199fcc441a7aee8427a7935243af8f152d6758124642",
   "advanced/voting-system.bug at O0": "e49f6349c9c8fc5d3e14cd737a860891139518b0e0ea73d3c01f314a60e80a30",
   "advanced/voting-system.bug at O1": "32f9c89a0890e1591b4e1aa29ec2b52d83a13c6ed06c4fc3391245002c505e38",
   "advanced/voting-system.bug at O2": "3479679f427cb43fcc7288d34666f865d94c20a05f216f719f41a218624823ec",

packages/bugc/src/optimizer/steps/common-subexpression-elimination.test.ts

@@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest";
 
 import { compile } from "#compiler";
 import type * as Ir from "#ir";
+import { executeProgram } from "#test/evm/behavioral";
 
 describe("CommonSubexpressionEliminationStep", () => {
   // `msg.value` before and after a call that is not inlined (it
@@ -38,4 +39,33 @@ code { r = g(3); }`;
       expect(values).toHaveLength(2);
     });
   }
+
+  // `msg.sender` twice: the second one is a call's argument, so the
+  // call must take the first one's value in its place
+  const callArgument = `name CallArg;
+define {
+  function f(a: address, n: uint256) -> address {
+    if (n == 0) { return a; }
+    return f(a, n - 1);
+  };
+}
+storage { [0] first: address; [1] passed: address; }
+create {}
+code {
+  first = msg.sender;
+  passed = f(msg.sender, 2);
+}`;
+
+  for (const level of [2, 3] as const) {
+    it(`replaces a call's argument (level ${level})`, async () => {
+      const result = await executeProgram(callArgument, {
+        calldata: "",
+        optimizationLevel: level,
+      });
+      expect(result.callSuccess).toBe(true);
+      const first = await result.getStorage(0n);
+      expect(first).not.toBe(0n);
+      expect(await result.getStorage(1n)).toBe(first);
+    });
+  }
 });

packages/bugc/src/optimizer/steps/common-subexpression-elimination.ts

@@ -179,6 +179,11 @@ export class CommonSubexpressionEliminationStep extends BaseOptimizationStep {
             block.terminator.value,
             globalReplacements,
           );
+        } else if (block.terminator.kind === "call") {
+          block.terminator.arguments = block.terminator.arguments.map(
+            (argument) =>
+              this.applyValueReplacement(argument, globalReplacements),
+          );
         }
       }
     });

packages/bugc/src/optimizer/steps/return-merging.test.ts

@@ -3,7 +3,9 @@ import { describe, it, expect } from "vitest";
 import * as Irgen from "#irgen";
 import { parse } from "#parser";
 import { Result } from "#result";
+import { compile } from "#compiler";
 import * as TypeChecker from "#typechecker";
+import { executeProgram } from "#test/evm/behavioral";
 
 import { ReturnMergingStep } from "./return-merging.js";
 import { OptimizationContextImpl } from "../optimizer.js";
@@ -198,4 +200,92 @@ describe("ReturnMergingStep", () => {
     // Should not merge returns with different values
     expect(returnBlocksAfter).toBe(returnBlocksBefore);
   });
+
+  // A call's continuation is entered only from its call, so it is
+  // neither removed nor entered by a jump. Each `return;` here is a
+  // block of its own; the order puts the continuation after, then
+  // before, the other return. The callee returns a value or nothing
+  // (a call with no `dest`)
+  const callees = {
+    value: `function f(n: uint256) -> uint256 {
+    if (n == 0) { return 0; }
+    r = r + 1;
+    return f(n - 1);
+  };`,
+    void: `function f(n: uint256) {
+    if (n == 0) { return; }
+    r = r + 1;
+    f(n - 1);
+  };`,
+  };
+  const program = (callee: string, body: string) => `name ReturnAfterCall;
+define {
+  ${callee}
+}
+storage { [0] r: uint256; }
+create {}
+code {
+${body}
+}`;
+  const variants = {
+    "after the other return": `
+  if (msg.data.length == 0) {
+    return;
+  }
+  f(2);
+  return;`,
+    "before the other return": `
+  if (msg.data.length != 0) {
+    f(2);
+    return;
+  }
+  return;`,
+  };
+
+  for (const [name, body] of Object.entries(variants)) {
+    for (const [returns, callee] of Object.entries(callees)) {
+      it(`keeps a call's continuation apart (${name}, ${returns})`, async () => {
+        const source = program(callee, body);
+
+        const compiled = await compile({
+          to: "ir",
+          source,
+          optimizer: { level: 3 },
+        });
+        if (!compiled.success) throw new Error("compile failed");
+        const { blocks } = compiled.value.ir.main;
+        const continuations = [...blocks.values()].flatMap((block) =>
+          block.terminator.kind === "call"
+            ? [block.terminator.continuation]
+            : [],
+        );
+        expect(continuations).not.toHaveLength(0);
+        for (const continuation of continuations) {
+          expect(blocks.has(continuation)).toBe(true);
+          for (const block of blocks.values()) {
+            const { terminator } = block;
+            const targets =
+              terminator.kind === "jump"
+                ? [terminator.target]
+                : terminator.kind === "branch"
+                  ? [terminator.trueTarget, terminator.falseTarget]
+                  : [];
+            expect(targets).not.toContain(continuation);
+          }
+        }
+
+        for (const [calldata, expected] of [
+          ["", 0n],
+          ["0x01", 2n],
+        ] as const) {
+          const result = await executeProgram(source, {
+            calldata,
+            optimizationLevel: 3,
+          });
+          expect(result.callSuccess).toBe(true);
+          expect(await result.getStorage(0n)).toBe(expected);
+        }
+      });
+    }
+  }
 });

packages/bugc/src/optimizer/steps/return-merging.ts

@@ -15,7 +15,17 @@ export class ReturnMergingStep extends BaseOptimizationStep {
       // Group blocks by their return value (or lack thereof)
       const returnGroups = new Map<string, string[]>(); // signature -> block IDs
 
+      // A call's continuation is entered only from its call, with the
+      // callee's return value on the stack, so it is never merged
+      const continuations = new Set<string>();
+      for (const block of func.blocks.values()) {
+        if (block.terminator.kind === "call") {
+          continuations.add(block.terminator.continuation);
+        }
+      }
+
       for (const [blockId, block] of func.blocks) {
+        if (continuations.has(blockId)) continue;
         if (block.terminator.kind === "return") {
           // Create a signature for the return statement
           const signature = this.getReturnSignature(block.terminator.value);