Skip to content

[LLHD] Add llhd.resample: pin event-trigger samples to their check block#10794

Open
AmurG wants to merge 1 commit into
llvm:mainfrom
AmurG:ripe/llhd-resample
Open

[LLHD] Add llhd.resample: pin event-trigger samples to their check block#10794
AmurG wants to merge 1 commit into
llvm:mainfrom
AmurG:ripe/llhd-resample

Conversation

@AmurG

@AmurG AmurG commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Consecutive edge-sensitive event waits in one process misfire: the second and later waits consume on the first wake after arming, including on the wrong edge.

module top;
  bit clk;
  always #5 clk = ~clk;
  initial begin
    @(negedge clk);   // expect t=10
    @(negedge clk);   // expect t=20
  end
endmodule

circt-verilog --ir-hw emits, per wait, a check block whose negedge trigger is comb.and(%before, comb.xor(%after, true)), where %after is the live sample, captured from outside the process (a module-level probe). The polarity term comb.xor(%after, true) is pure and identical across the two check blocks, so CSE merges it into the first one:

^bb2(%10: i1):                              // first check block
  %11 = comb.xor bin %8, %true : i1
  %12 = comb.and bin %10, %11 : i1
  ...
^bb5(%18: i1):                              // second check block
  %19 = comb.and bin %18, %11 : i1          // reuses %11 -- stale sample

The merged term is live across the suspension, gets persisted by the coroutine lowering, and every later wait then tests the first wake's clk sample: a stale 0 makes the negedge term true at a posedge.

A blanket per-resume-block rematerialization would be unsound: a genuine pre-wait local temp (x = f(port); @(clk); if (x)) must keep its arm-time value and is indistinguishable post-CSE from the merged artifact. The distinction only exists at creation time, so this materializes it there as a dedicated op, the modeling direction requested in #10624:

  • llhd.resample is an identity carrying a MemRead effect (same rationale as llhd.current_time). CSE merges read-only ops only within a single block, so samples pinned in different check blocks can never merge. No folder, deliberately: the barrier must survive the greedy driver.
  • MooreToCore wraps the live "after" sample for edge-sensitive detects; the LSB projection (IEEE 1800-2017 9.4.2) moves downstream of the marker so wide-signal edge chains are covered.
  • arc-lower-processes and arc-lower-coroutines fold the markers to their operand once per-block renaming / suspension-capture threading has made the freshness structural. Deseq sees through markers; HoistSignals treats them as reads.

Tested: resample.mlir (round-trip, CSE resistance), lower-processes-resample.mlir (fold-after-renaming contract), deseq.mlir / hoist-signals.mlir transparency cases, basic.mlir wait_event updates.

Written with AI assistance; I've reviewed the diff and tests.

Consecutive edge-sensitive event waits in one process misfire: with two
@(negedge clk) waits in sequence, the second and later waits consume on
the FIRST wake after arming, including on posedges. The root cause is
visible in the IR of a 20-line reproducer on current main: MooreToCore
emits, per wait, a check block whose negedge trigger is
`comb.and(%before, comb.xor(%after, true))`, where %after is the live
sample -- frequently a value captured from outside the process (a
hoisted probe or module port). The polarity term `comb.xor(%after,
true)` is pure and IDENTICAL across consecutive negedge check blocks,
so CSE merges them into the first check block. The merged term is live
across the suspension, gets persisted by the coroutine lowering, and
every later wait then tests the FIRST wake's sample: a stale-0 clk
sample makes the "new value must be 0" negedge term true at a posedge.

A blanket per-resume-block rematerialization would be unsound: a
genuine pre-wait local temp `x = f(port); @(clk); if (x)` must keep its
arm-time value and is indistinguishable post-CSE from the merged
artifact. The distinction only exists at creation time, so materialize
it there:

- New `llhd.resample` op: an identity carrying a MemRead side effect
  (same rationale as `llhd.current_time`). MLIR CSE merges read-only
  ops only within a single block, so samples pinned in different check
  blocks can never merge, and every derived pure chain stays rooted in
  a block-local op. No folder on the op by design -- the greedy driver
  must never erase the barrier.
- MooreToCore's computeTrigger materializes the "after" sample first
  and wraps it in `llhd.resample` for edge-sensitive detects (anychange
  triggers compare against the per-block "before" operand and cannot
  merge). The LSB projection (IEEE 1800-2017 9.4.2) now happens at comb
  level, downstream of the marker, so wide-signal edge chains are
  covered too.
- arc-lower-processes folds the markers to their operand right after
  the per-argument renaming has bound each block's uses to that block's
  re-entry values: from that point the freshness is structural
  (per-block roots differ, CSE cannot re-merge). arc-lower-coroutines
  folds any remaining markers after threading captured values, so no
  marker survives to LowerArcToLLVM.
- Deseq sees through markers (getValueField unwrap plus the side-effect
  exemption) and HoistSignals treats them as reads, so promotion and
  probe hoisting are unaffected.

Tests: resample.mlir (round-trip, CSE resistance in-block and
cross-block), lower-processes-resample.mlir (fold-after-renaming
contract), deseq.mlir and hoist-signals.mlir transparency cases, and
updated basic.mlir wait_event checks pinning the marker emission.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@circt-bot

circt-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

Results of circt-tests run for d5f65f9 compared to results for 01b5614: no change to test results.

@fabianschuiki

Copy link
Copy Markdown
Contributor

Yikes, this is an excellent find 👍. And I'm pretty sure this issue of CSE applying across suspending ops also happens at the Moore dialect level. I'm pretty sure this is down to the current process IR ops "abusing" SSA semantics: you'd think an SSA value has one definition, and one value. But with wait/suspension ops and process-external SSA values being captured, those SSA values can magically change -- which violates the SSA invariant. I'm not entirely sure how we'll want to fix this -- pretty certain though that we don't want to add an op to make up for an ill-designed process op. One way to deal with this would be to go the arc.coroutine route: make processes IsolatedFromAbove and force them to capture external SSA values as operands which are exposed as block arguments internally, with the entry block and any successor blocks to suspension ops carrying these block arguments. I think that's clean from an SSA point of view, but it's also somewhat challenging to construct since a single SSA value outside the process will have multiple definitions inside the process (one of multiple block args depending on which block you're resuming at), and you need a fairly annoying domination frontier walk to know where to add destination operands to converging control flow. But maybe that's just complexity we need to deal with.

Btw, this is exactly the kind of problem where an LLM agent just fudges stuff around until the tests somehow pass, even though there's a fundamental design flaw in the code base that needs fixing instead.

@fabianschuiki

Copy link
Copy Markdown
Contributor

You can get a similar issue in the Moore dialect:

module top;
  bit x, y;
  initial flop(x, y);
endmodule

task flop(input bit x, output bit y);
  y = ~x;
  #1ns;
  y = ~x;
endtask

Running this through circt-verilog --ir-moore yields:

// ...
moore.coroutine private @flop(%arg0: !moore.i1, %arg1: !moore.ref<i1>) {
  %0 = moore.constant_time 1000000 fs
  %1 = moore.not %arg0 : i1
  moore.blocking_assign %arg1, %1 : i1
  moore.wait_delay %0
  moore.blocking_assign %arg1, %1 : i1 // <--- same problem
  moore.return
}

😢

@AmurG

AmurG commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — agreed on the root cause. The process/coroutine ops let anything that samples mutable state stay live across a suspension, and at that point every value-numbering transform (CSE here, hoisting just as much) is licensed to break it. llhd.resample doesn't repair that contract — it gives the passes that create cross-suspension samples (trigger computation in MooreToCore, coroutine lowering) a way to say "this sample belongs to this check block" explicitly. So I'd describe it as making one load-bearing case sound, not as the fix for the process-op design.

Small note on the task example: I think that exact one is legal — x is an input arg, so it's copied in at call time (1800-2017 13.5) and the second y = ~x is supposed to observe the call-time value; reusing %1 there is fine. The trap variant is a ref arg or a direct module-scope read in the task body: those must re-observe, and two moore.reads separated only by a wait_delay (which models no write effect) can still be merged today.

On the IsolatedFromAbove design: agreed that's the clean end state, and I don't think the domination-frontier walk needs to be hand-rolled. If each captured value is treated as a promotable slot — capture = load, every suspension = clobber — then placing the re-capture block arguments is ordinary SSA construction, i.e. mem2reg where suspension ops act as stores. llhd.wait is already a terminator with an explicit destination block, so it can carry re-captured values as destination operands naturally, and the converging-control-flow cases fall out of the standard algorithm instead of a bespoke walk. The real cost lands on construction in ImportVerilog/MooreToCore, which currently build processes non-isolated from the start (and the Moore coroutine ops would want the same treatment, per your example).

Happy to go either way with this PR: park it and fold the repros into the redesign's test set, or keep it as a self-contained interim guard until the isolated-process work lands — it's easy to delete later. We have suite-scale coverage on the fork branch that exposes the miscompile, so I can prototype the block-arg capture approach against that and report back with data if that's useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants