Only preempt simulator testbenches on explicit wait points

[whitequark]

Before this commit, testbenches (generators added with add_testbench) were not only preemptible after any yield, but were guaranteed to be preempted by another testbench after every yield. This is evil: if you have any race condition between testbenches, which is common, this scheduling strategy will maximize the resulting nondeterminism by interleaving your testbench with every other one as much as possible. This behavior is an outcome of the way add_testbench is implemented, which is by yielding Settle() after every command.

One can observe that:

• yield value_like should never preempt;
• yield assignable.eq() in add_process() should not preempt, since it only sets a next signal state, or appends to write_queue of a memory state, and never wakes up processes;
• yield assignable.eq() in add_testbench() should only preempt if changing assignable wakes up an RTL process. (It could potentially also preempt if that wakes up another testbench, but this has no benefit and requires sim.set() from RFC 36 to be awaitable, which is not desirable.)

After this commit, PySimEngine._step() is implemented with two nested loops instead of one. The outer loop iterates through every testbench and runs it until an explicit wait point (Settle(), Delay(), or Tick()), terminating when no testbenches are runnable. The inner loop is the usual eval/commit loop, running whenever a testbench changes design state.

PySimEngine._processes is a set, which doesn't have a deterministic iteration order. This does not matter for processes, where determinism is guaranteed by the eval/commit loop, but causes racy testbenches to pass or fail nondeterministically (in practice depending on the memory layout of the Python process). While it is best to not have races in the testbenches, this commit makes PySimEngine._testbenches a list, making the outcome of a race deterministic, and enabling a hacky work- around to make them work: reordering calls to add_testbench().

A potential future improvement is a simulation mode that, instead, randomizes the scheduling of testbenches, exposing race conditions early.

---

Needs:

• A test

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.80%. Comparing base (9ed83b6) to head (c9ab69e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1231      +/-   ##
==========================================
+ Coverage   89.78%   89.80%   +0.02%     
==========================================
  Files          43       43              
  Lines        9923     9923              
  Branches     2395     2400       +5     
==========================================
+ Hits         8909     8911       +2     
+ Misses        822      820       -2     
  Partials      192      192              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stafverhaegen-chipflow]

A potential future improvement is a simulation mode that, instead, randomizes the scheduling of testbenches, exposing race conditions early.

To me this is a requirement to do in the future and not optional. One of the problems with classic RTL simulation was that by trying to put determinism into situations where there is a race in the design that the problem was not seen during simulation but that the race actually shows up on a real chip.
Therefor in classical flow a post-layout verification step is normally included where simulations are done with back-annotated timing data to catch such situations before actually going to tape-out. I think we have to avoid that for a Amaranth based ASIC flow.

[whitequark]

To me this is a requirement to do in the future and not optional. One of the problems with classic RTL simulation was that by trying to put determinism into situations where there is a race in the design that the problem was not seen during simulation but that the race actually shows up on a real chip.
Therefor in classical flow a post-layout verification step is normally included where simulations are done with back-annotated timing data to catch such situations before actually going to tape-out. I think we have to avoid that for a Amaranth based ASIC flow.

Are we talking about the same thing? The testbenches are very explicitly not a part of the design; they are a part of the test suite. There is nothing to back-annotate because testbenches are behavioral and can only be used to provide stimulus and simulate I/O.

[stafverhaegen-chipflow]

What I had in the back of my mind is the use of behavioral models for RTL blocks or full-custom blocks for speed reasons.

[whitequark]

What I had in the back of my mind is the use of behavioral models for RTL blocks or full-custom blocks for speed reasons.

These will not be using add_testbench but add_process and add_process cannot race. (This is the entire reason for having add_process.)

[stafverhaegen-chipflow]

Is await sim.tick().sample(...) allowed in add_testbench behavioral models ?
One may need non-propagated signals then also in these processes.

[whitequark]

Is await sim.tick().sample(...) allowed in add_testbench behavioral models ?

Yes, await sim.tick() and everything you hang off it works exactly the same in add_process and add_testbench.

[stafverhaegen-chipflow]

Then I agree add_testbench is OK for these behavioral models.