Mocks of combinational circuits and `add_process`

[lekcyjna123]

Hi,
recently there was implemented RFC 27 and now the yield Settle() in add_process is deprecated. I would like to ask how in the new way of testing, mocks for external combinational circuits should be implemented. As I understand mocks are behavioural replacement so according to RFC 27 they should be implemented with add_process. But in case of combinational circuits there is a race. Here is an example:

from amaranth import *
from amaranth.sim import *


class DUT(Elaboratable):
    def __init__(self):
        self.in1 = Signal(8)
        self.in2 = Signal(8)
        self.out = Signal(8)

        # Signals used to communicate with external module, which
        # isn't a submodule of this Elaboratable.
        self._middle_in = Signal()
        self._middle_out = Signal(8)

    def elaborate(self, platform):
        m = Module()

        # Set input to the external module
        m.d.comb += self._middle_in.eq( (self.in1 + self.in2).any())

        # Process output from the external module in the same cycle
        m.d.sync += self.out.eq(self._middle_out)

        return m

def test_case():
    circ = DUT()

    def proc():
        yield Passive()
        while True:
            yield Settle() # without Settle test fails
            middle_in = yield circ._middle_in
            out = 0
            if middle_in:
                out = 7
            yield circ._middle_out.eq(out)
            yield Tick()

    def testbench():
        yield circ.in1.eq(3)
        yield circ.in2.eq(4)
        yield Tick()
        out = yield circ.out
        assert out == 7

    sim = Simulator(circ)
    sim.add_clock(1e-6)
    sim.add_process(proc)
    sim.add_testbench(testbench)
    sim.run()

The example is based on the fact that first inputs to external module has to be calculated and after that we can safely execute body of proc.

The yield Settle can be removed if proc will be changed to add_testbench, but according to amaranth-lang/rfcs#36 (comment) there are doubts if testbenches should be allowed to be passive. Whats more this doesn't scale up. If we add a second external module in the chain, then testbenches also stops to work:

from amaranth import *
from amaranth.sim import *


class DUT(Elaboratable):
    def __init__(self):
        self.in1 = Signal(8)
        self.in2 = Signal(8)
        self.out = Signal(8)

        # Signals used to communicate with external module, which
        # isn't a submodule of this Elaboratable.
        self._middle_in = Signal()
        self._middle_out = Signal(8)

        # Signals used to communicate with second external module, which
        # isn't a submodule of this Elaboratable.
        self._middle_in_2 = Signal()
        self._middle_out_2 = Signal(8)

    def elaborate(self, platform):
        m = Module()

        # Set input to the external module
        m.d.comb += self._middle_in.eq( (self.in1 + self.in2).any())

        # Set input to the second external module
        m.d.comb += self._middle_in_2.eq( self._middle_out.any() )

        # Process output from the external module in the same cycle
        m.d.sync += self.out.eq(self._middle_out + self._middle_out_2)

        return m

def test_case():
    circ = DUT()

    def proc():
        yield Passive()
        while True:
            yield Settle()
            middle_in = yield circ._middle_in
            out = 0
            if middle_in:
                out = 7
            yield circ._middle_out.eq(out)
            yield Tick()

    def proc2():
        yield Passive()
        while True:
            yield Settle()
            yield Settle() 
            middle_in = yield circ._middle_in_2
            out = 0
            if middle_in:
                out = 10
            yield circ._middle_out_2.eq(out)
            yield Tick()

    def testbench():
        yield circ.in1.eq(3)
        yield circ.in2.eq(4)
        yield Tick()
        out = yield circ.out
        assert out == 17

    sim = Simulator(circ)
    sim.add_clock(1e-6)
    # If both `add_process` below are changed to `add_testbench` the race will occure
    sim.add_process(proc)
    sim.add_process(proc2)
    sim.add_testbench(testbench)
    sim.run()

What is the current way of mocking external combinational stuff after RFC 27?

[whitequark]

I would like to ask how in the new way of testing, mocks for external combinational circuits should be implemented. As I understand mocks are behavioural replacement so according to RFC 27 they should be implemented with add_process. But in case of combinational circuits there is a race. Here is an example:

Sorry, I am confused about your example. You say that you are simulating a combinational circuit, but you are using yield Tick(). yield Tick() is used for simulating a synchronous circuit. Could you explain this please?

[whitequark]

        while True:
            yield Settle()
            yield Settle() 

Wait, is this code pattern why you wanted yield TrueSettle()?

[lekcyjna123]

You say that you are simulating a combinational circuit, but you are using yield Tick()

Sorry, for introducing confusion. My example should present a scenario where the simulated circuit is synchronous, but it uses some external combinational modules. In unit tests such external modules should be substituted by mock processes to verify only interesting module. In my example proc and proc2 are the mocks. And I am wondering what is correct way to write them.

The presented example is very artificial, but I wanted to keep it simple and minimal. In reality in transactron we have modules which takes Methods. From high level point of view Method is a function which can be executed in context of other Module (e.g. we can call write in Fifo to put a new element to it). The decision if method should be executed or not is taken in runtime using combinational circuits. In unit tests we inject Method mocks into elaboratable under test, and these mocks are next handled by testing processes. So the idea behind the presented example is that proc and proc2 are mocks of methods which are chosen in runtime if they should be executed or not, and DUT is a module which calls external methods.

        while True:
            yield Settle()
            yield Settle() 

Wait, is this code pattern why you wanted yield TrueSettle()?

Roughly yes. Using yield Settle() we introduce execution order of mocks, so that we are sure that in case of combinational mocks which depends one of the other there will be executed correctly. Sadly this cause that in generic code we can not use yield Settle() to wait till all signals are already stabilized.

If you want to see real, not minimalized example, you can see on TestMethodFilter:
https://github.com/kuznia-rdzeni/coreblocks/blob/0ae1ed8902d5b0749651c28b3e274a79b16a8096/test/transactions/test_transaction_lib.py#L436
which tests MethodFilter (equivalent of functional filter but in hardware)
https://github.com/kuznia-rdzeni/coreblocks/blob/0ae1ed8902d5b0749651c28b3e274a79b16a8096/transactron/lib/transformers.py#L120

MethodFilter takes a Method which implements a condition and if it is true it calls target method. Everything happens combinationaly in the same cycle. So when we mock methods in TestMethodFilter then cmeth mock has to be executed first and after that the target mock can be executed (because it depends on cmeth results). This order we provides using correct number of yield Settle() which are hidden under sched_prio in def_method_mock. Under the hood sched_prio=2 is equal to the

yield Settle()
yield Settle()
yield Settle()

(One more yield Settle() is needed to guarantee that inputs to the sched_prio=0 are already set to correct value).

[lekcyjna123]

I see in amaranth-lang/rfcs#36 proposition to introduce sim.changed. I think that this will solve the issue because yield Tick() in proc and proc2 will be substituted by sim.changed, so there will be possible to execute the mock multiple times in the one cycle (so the order of process execution will not matter) and if needed simulation will go to the next cycle autonomously.

[whitequark]

RFC 36 has been merged. Does it fit your use case?

[lekcyjna123]

Yes. It should fit and I see that #1213 is added to 0.5 milestone so we will be available in the same version as RFC 27, so there will be no problem in transition time. Thanks!