Add an RFC for async testbench functions.

[zyp]

Rendered

[whitequark]

I'm de-nominating this RFC since it depends on #27 which isn't ready yet.

[zyp]

Updated to match the changes in #27.

[whitequark]

We have discussed this RFC on the 2024-02-26 weekly meeting. Several concerns were raised about the RFC:

• (@wanda-phi) Instead of await signal.get() and await signal.set() methods that are implemented by a Value or a ValueCastable, we should have await sim.get(signal) and await sim.set(signal, value) that use type conversion methods for ValueCastable. For converting a high-level representation to a bit pattern, we have ValueCastable.const; we do not have anything that goes in the other direction at the moment.
  • These type conversion methods will be necessary anyway for implementing simulation behavior for shaped lib.memory.Memory, as signal.get() and signal.set() are not usable in that context (there is no underlying Signal to set or get in the implementation of ValueCastable.set()).
• (@whitequark) delay(None) should be prohibited.
• (@whitequark) We should consider different naming for active/passive.
• (@whitequark) The sim argument should be passed at all times when an async function is provided to add_process/add_testbench, since the upgrade path is easy and usefulness otherwise is very limited.
• (@whitequark, @wanda-phi) tick(domain="sync") should have some way to name a domain that is not in the topmost module.
  • (@whitequark) Proposed API: tick(domain="sync", *, context=Elaboratable | None), and fragment origins are used to find out which clock domain object within context the domain argument names.
  • (@wanda-phi) It is error-prone to have context default to None and we should forbid using strings for domain and instead have an API tick(LateBoundDomain(elaboratable, "sync")).
  • (@whitequark) We have a pending clock domain redesign in 0.6, which will almost certainly affect any proposed API here, so time spent on it now is probably wasted.

[whitequark]

I think this should add a function run_async that causes any awaits to be forwarded to an external event loop, like from asyncio.

[zyp]

I think that is a good idea, but orthogonal enough to this RFC that it deserves its own RFC. I can write it if you'd like.

[whitequark]

Sounds good to me!

[whitequark]

time()
Return the current simulation time.

What is the type of the returned value?

[zyp]

Presumably seconds as a float, for consistency, since that's what's used in e.g. add_clock()'s period argument. I'm open for other suggestions though.

[whitequark]

Presumably seconds as a float, for consistency

Using float in add_clock was a bad decision and we should probably not multiply it if we can avoid it...

[zyp]

In that case I propose we change all simulation time units to seconds as a Fraction.

[whitequark]

Sounds good to me. It should internally use femtoseconds though.

[zyp]

Shouldn't what's used internally be an implementation detail left to the simulation engine? That said, using femtoseconds would throw away the benefit of using a Fraction since it's potentially lossy. The period of e.g. 30 MHz is not an integer femtoseconds.

Is it worth using a more complicated type than float if it might be lossy anyway? (FWIW, a Python float gives femtosecond resolution up to nine seconds of simulation time or so.)

[whitequark]

CXXRTL uses femtoseconds internally (with a custom data type wrapping them into a integral seconds plus fractional second as femtoseconds) and I'd like the rounding error to match between Python and C++.

[whitequark]

Some comments in advance of the Monday meeting.

As an example, let's consider a simple stream interface with valid, ready and data members.

This example has the glaring issue of not having any way of specifying the clock domain at all. This needs to be called out in the RFC text as an avenue for future work; i.e. that the example isn't the outcome directly enabled by the RFC but a desirable future result.

A better solution perhaps is to reframe these functions as not methods on the streams themselves but as helpers that could be eventually put onto the streams. Then you can just have the clock domain as an argument (or ignore it like I did in some single-clock code I worked on recently) and the example motivates the RFC well.

add_process(process, *, passive=False)

Should add_process not have passive=True by default? In our model, processes are going to be reserved exclusively for behavioral replacement of logic, and simulated logic is functionally passive.

Alternatives:

1. no default passive= argument for add_process and value must have specified explicitly;
2. add_process() is only capable of creating passive-by-default processes.

I particularly like the 2nd alternative since it further cements the distinction in the application of the two APIs, which I would like to make as clear as possible.

add_testbench(process, *, passive=False)

It's not completely clear to me, but when do we want passive testbenches?

If we remove passive from both add_process and add_testbench we end up with a much simpler API conceptually:

• add_process replaces some logic behaviorally. In most cases it can be shut down if no testbench is running, however in recognition that e.g. an AXI transaction monitor acts as-if it was logic attached to the bus, but needs to ensure that any transaction proceeds to completion, it can temporarily prevent the simulation from being shut down.

The async function must accept a named argument sim, which will be passed a simulator context.

Now that we've amended the RFC to have a different API for legacy and async processes we can pass the argument positionally.

get(signal)

This does not just work on signals, right? I think the actual signature is best described by the two overloads:

• get(expr: Value) -> int
• get(expr: ValueCastable) -> any

Returns the value of signal when awaited. When signal is a value-castable, the value will be converted through .from_bits(). (Pending RFC #51)

RFC #51 has been accepted.

set(signal, value)

Similarly I would propose:

• set(expr: Value, value: ConstLike)
• set(expr: ValueCastable, value: any)

(Emphasis on argument names here.)

delay(interval)

I think this needs a better type specification. I don't recall what we currently support so I'll leave it as-is.

Should delay() be allowed in add_process()? (I think yes; otherwise you could not e.g. simulate a phase shifter, which seems desirable.)

tick(domain="sync", *, context=None)

There are two significant design issues with tick (and Tick on which it is based). Consider what happens when you call await tick("domain") and the domain has its async reset triggered? As it stands, the await call returns.

The design issues are:

• The name tick (or the clocked replacement I proposed earlier) evokes the idea that it triggers on a clock edge, but it triggers on reset edge too.
• There is no straightforward way to distinguish the two cases between each other.

Some strawman proposals I can come up with:

• Forbid tick() from triggering on async reset and if this ever happens in a simulation, crash all the waiting processes. (Very rude.)
• Add a parameter to tick() declaring whether an async reset edge is expected, crashing the process as above if it's not specified or False.
• Make the await sim.tick() call return True if the domain was async-reset, False otherwise.
  • The same but any domain reset counts.
  • This can be ignored too easily.
• Make the call return a enum or something so that you have to use if await sim.tick() == BikeshedEnum.Clock: (the precedence does work out correctly).
  • Make the BikeshedEnum a MustUse class.
  • Only do this by opt-in.
  • Use two functions, one async-reset-aware and one async-reset-unaware.

changed(signal, value=None)

We probably need some way to trigger on up to two signals with specific values (for sync processes), or an arbitrary number of values without specific values (for comb processes).

Strawman proposals:

• changed((signal1, signal2, ...), value=(value1, value2, ...)); a straightforward extension of the current API.
  • Ugly.
• changed(signal, value=None).or_changed(signal, value=None); chaining like with until
  • Really verbose and ugly for comb processes.
  • Creates what is essentially a simulation-specific sub-language with entirely new syntax and unclear semantics.
• changed(signal) or changed(signal == value) or changed((signal1 == value1) | (signal2 == value2))
  • Worse syntax than Verilog.
  • Doesn't even match semantics of what's happening.
• changed(signal, ...) or posedge(signal, ...) or negedge(signal, ...), chainable
  • Composable, clearly defined semantics that doesn't borrow syntax from the core language in a misleading way.
  • Obnoxious for processes generic over edge type, without an obvious way to solve this as long as multiple signals can be provided. (edge((signal, 1), ...) has all the problems of the first option.)
• changed(signal, ...) (without allowing a value) + tick() (without any way to trigger on an edge that doesn't involve a domain)
  • Seems limiting, as e.g. a DDR register implementation may need to trigger on a clock both ways.

I think my final proposal is a variation on some of the above, namely the combination of:

• async for values in sim.changed(signals: Iterable[Signal]): perfect for implementing comb process replacements, allows the simulator to amortize the cost of waiting on signals by registering event listeners and leaving them there.
• await sim.edge(clk, 1).edge(rst, 0): perfect for implementing sync processes or variations on the theme, like DDR buffers.
  • Could have sim.posedge(clk) as an alias.
  • First argument must be a 1-bit signal or a 1-bit slice of a signal.
  • Returns the values of all registered triggers.

Examples:

1. Combinational adder as a process:

   a = Signal(); b = Signal(); o = Signal()
   async def adder(sim):
       async for a_val, b_val in sim.changed(a, b):
           await sim.set(o, a_val + b_val)
   sim.add_process(adder)

2. DDR IO buffer as a process:

   o = Signal(2); pin = Signal()
   async def ddr_buffer(sim):
       while True: # could be extended to pre-capture next `o` on posedge
           await sim.negedge()
           await sim.set(pin, o[0])
           await sim.posedge()
           await sim.set(pin, o[1])
   sim.add_process(ddr_buffer)

3. Flop with configurable edge reset and posedge clock as a process:

   clk = Signal(); rst = Signal(); d = Signal(); q = Signal()
   def dff(rst_edge):
       async def process(sim):
           while True:
               clk_val, rst_val = await sim.posedge(clk).edge(rst, rst_edge)
               await sim.set(q, 0 if rst_val == rst_edge else await sim.get(d))
       return process
   sim.add_process(dff(rst_edge=0))

active()

Proposed new name: critical(). This is a name that will catch the eye of the reader, but that's as it should: it's not normal at all for add_process to block the simulation from exiting. Downside: "critical section" isn't widely used in Python parlance.

Alternative: lock(). Downside: too generic.

time()

Given the lack of consensus on what it should return in the current codebase (with the proposed solution being a new type, amaranth.sim.Time or something like that), I propose descoping this feature from this RFC. There's plenty to go over already, and I think it's quite critical that we ship this soon.

It should be possible to combine triggers

Addressed in this comment.

(@wanda-phi) sim.memory_read(memory, address), sim.memory_write(memory, address, value[, mask])?

Seems OK to me to include with this name. People who use autocompletion on sim. may get confused but they'll also get confused by sim.memory_instance_read, and adding a type annotation on the first argument (which should be instance: MemoryInstance) should do much to address this confusion.

sim deals with core types, not library types, after all.

Maybe a way to skip a given number of triggers? We still lack a way to say «advance by n cycles».

This is trivial to work around with the current syntax:

for _ in range(5): await sim.tick()

Adding syntax for this would be an optimization, but pysim doesn't need it and cxxsim isn't anywhere near the stage where it can be used. I think this concern can be safely ignored in this RFC (and moved to "Future work").

[wanda-phi]

General comment: a new simulation interface also needs to be defined for lib.memory.Memory, wrapping the sim.memory_* primitives.

[wanda-phi]

After thinking a bit on the .tick() combinator interactions, we find the complexity of AsyncReset handling concerning. We likewise find the .until() interaction with combinators to be somewhat complex.

Since tick() is unlikely to be combined with other combinators, we are considering the following design:

• .delay(), .edge() with its variants, and .changed() remain as combinable trigger objects; they cannot [?] be used with .until(); they provide a low-level interface to create weird primitives
• .tick() cannot be combined with any other trigger, including another .tick(); however, it can be combined with .until() and .repeat(num: int); it provides a high-level interface to interact with a single clock domain

[wanda-phi]

I'd also like to again raise the point that I brought up on the last meeting, and that apparently got lost.

We have effectively two kinds of processes, in which sim awaitables look the same, but have different semantics. We intend that users create helper simulation functions on elaboratables and whatnot that call these awaitables. It is overwhelmingly likely that any given function will be designed to work on only one of the two kinds of process, and it is likely that someone will accidentally call such a function from the wrong kind of process, and end up with incomprehensible bugs. How do we prevent such a mistake?

[whitequark]

We have discussed this RFC on the 2024-03-18 weekly meeting. The disposition was to merge. Unresolved questions were resolved as follows:

• Use posedge and negedge names (over pos_edge and neg_edge).