Incorrect generated MyHDL code

[josyb]

The application generates this code:

        crcOut[0].next = crcIn[5] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[5] ^ data[7] ^ data[8] ^ data[9]

but it must be:

        crcOut.next[0] = crcIn[5] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[5] ^ data[7] ^ data[8] ^ data[9]

you can distill some insights on this from: MyHDL: why sig[j:i].next = val can’t work

You fix it by changing:

        for i, bit in enumerate(word):
            ret.append(f"        {outCrcName}[{i}].next = {bit.gen_myhdl()}")

into:

        for i, bit in enumerate(word):
            ret.append(f"        {outCrcName}.next[{i}] = {bit.gen_myhdl()}")

[mbuesch]

Hi. Thanks a lot for your report.

I'm pretty sure that this used to work just fine with older versions of MyHDL. Therefore, I'm wondering why it has been classified as "can't work" now.

[mbuesch]

Ok, I now tried it and I'm not so sure anymore whether the proposed change is correct.

The only difference in the generated Verilog and VHDL code is that after the proposed change to the MyHDL code a process/always block is generated in VHDL/Verilog. But that's actually not what I think should be done. Currently, without this change, it generates fully combinatorial terms without processes and without registers (only wires). And that's actually what crcgen generates directly, too.

[josyb]

I could have been more complete: Your originally generated code will convert but not simulate. I added a testbench to your generated design:

from myhdl import block, always_comb

@block
def crc(crcIn, data, crcOut, CORRECTED_CODE=0):

    if CORRECTED_CODE:

        @always_comb
        def logic():
            crcOut.next[0] = crcIn[5] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[5] ^ data[7] ^ data[8] ^ data[9]
            crcOut.next[1] = crcIn[0] ^ crcIn[5] ^ crcIn[6] ^ crcIn[7] ^ data[0] ^ data[5] ^ data[6] ^ data[7]
            crcOut.next[2] = crcIn[0] ^ crcIn[1] ^ crcIn[6] ^ crcIn[7] ^ crcIn[8] ^ data[0] ^ data[1] ^ data[6] ^ data[7] ^ data[8]
            crcOut.next[3] = crcIn[1] ^ crcIn[2] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[1] ^ data[2] ^ data[7] ^ data[8] ^ data[9]
            crcOut.next[4] = crcIn[0] ^ crcIn[2] ^ crcIn[3] ^ crcIn[5] ^ crcIn[7] ^ data[0] ^ data[2] ^ data[3] ^ data[5] ^ data[7]
            crcOut.next[5] = crcIn[0] ^ crcIn[1] ^ crcIn[3] ^ crcIn[4] ^ crcIn[6] ^ crcIn[8] ^ data[0] ^ data[1] ^ data[3] ^ data[4] ^ data[6] ^ data[8]
            crcOut.next[6] = crcIn[1] ^ crcIn[2] ^ crcIn[4] ^ crcIn[5] ^ crcIn[7] ^ crcIn[9] ^ data[1] ^ data[2] ^ data[4] ^ data[5] ^ data[7] ^ data[9]
            crcOut.next[7] = crcIn[2] ^ crcIn[3] ^ crcIn[6] ^ crcIn[7] ^ crcIn[9] ^ data[2] ^ data[3] ^ data[6] ^ data[7] ^ data[9]
            crcOut.next[8] = crcIn[3] ^ crcIn[4] ^ crcIn[5] ^ crcIn[9] ^ data[3] ^ data[4] ^ data[5] ^ data[9]
            crcOut.next[9] = crcIn[4] ^ crcIn[6] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[4] ^ data[6] ^ data[7] ^ data[8] ^ data[9]

    else:

        @always_comb
        def logic():
            crcOut[0].next = crcIn[5] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[5] ^ data[7] ^ data[8] ^ data[9]
            crcOut[1].next = crcIn[0] ^ crcIn[5] ^ crcIn[6] ^ crcIn[7] ^ data[0] ^ data[5] ^ data[6] ^ data[7]
            crcOut[2].next = crcIn[0] ^ crcIn[1] ^ crcIn[6] ^ crcIn[7] ^ crcIn[8] ^ data[0] ^ data[1] ^ data[6] ^ data[7] ^ data[8]
            crcOut[3].next = crcIn[1] ^ crcIn[2] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[1] ^ data[2] ^ data[7] ^ data[8] ^ data[9]
            crcOut[4].next = crcIn[0] ^ crcIn[2] ^ crcIn[3] ^ crcIn[5] ^ crcIn[7] ^ data[0] ^ data[2] ^ data[3] ^ data[5] ^ data[7]
            crcOut[5].next = crcIn[0] ^ crcIn[1] ^ crcIn[3] ^ crcIn[4] ^ crcIn[6] ^ crcIn[8] ^ data[0] ^ data[1] ^ data[3] ^ data[4] ^ data[6] ^ data[8]
            crcOut[6].next = crcIn[1] ^ crcIn[2] ^ crcIn[4] ^ crcIn[5] ^ crcIn[7] ^ crcIn[9] ^ data[1] ^ data[2] ^ data[4] ^ data[5] ^ data[7] ^ data[9]
            crcOut[7].next = crcIn[2] ^ crcIn[3] ^ crcIn[6] ^ crcIn[7] ^ crcIn[9] ^ data[2] ^ data[3] ^ data[6] ^ data[7] ^ data[9]
            crcOut[8].next = crcIn[3] ^ crcIn[4] ^ crcIn[5] ^ crcIn[9] ^ data[3] ^ data[4] ^ data[5] ^ data[9]
            crcOut[9].next = crcIn[4] ^ crcIn[6] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[4] ^ data[6] ^ data[7] ^ data[8] ^ data[9]

    return logic

if __name__ == '__main__':
    import random
    random.seed('We want repeatable randomness')

    from myhdl import Signal, intbv, instance, delay, StopSimulation, instances

    @block
    def tb_crc10_10(CORRECTED_CODE):
        T_OPS = 32

        crcIn = Signal(intbv(0)[10:])
        data = Signal(intbv(0)[10:])
        crcOut = Signal(intbv(0)[10:])

        dut = crc(crcIn, data, crcOut, CORRECTED_CODE)
        dut.nam = 'crc_10_10'

        @instance
        def stimulus():
            crcIn.next = 0
            yield delay(50)
            for _ in range(T_OPS):
                data.next = random.randint(0, 2 ** 10 - 1)
                yield delay(10)
                crcIn.next = crcOut
            yield delay(50)

            raise StopSimulation

        return instances()

    def convert(CORRECTED_CODE):
        crcIn = Signal(intbv(0)[10:])
        data = Signal(intbv(0)[10:])
        crcOut = Signal(intbv(0)[10:])

        instance = crc(crcIn, data, crcOut, CORRECTED_CODE)
        instance.convert(hdl='Verilog', testbench=False, name='crc_{}'.format(cc))
        instance.convert(hdl='VHDL', name='crc_{}'.format(cc))

    for cc in [1, 0]:
        convert(cc)

        dft = tb_crc10_10(cc)
        dft.config_sim(trace=True, filename='tb_crc_{}'.format(cc))
        dft.run_sim()

the simulation with CORRECTED_CODE = 0 raises this:

Traceback (most recent call last):
  File "C:\myhdlsupport\workspace-MyHDL-support\issues\mbuesch\crc_10_10.py", line 114, in <module>
    dft.run_sim()
  File "C:\myhdlsupport\myhdl\myhdl\_block.py", line 353, in run_sim
    self.sim.run(duration, quiet)
  File "C:\myhdlsupport\myhdl\myhdl\_Simulation.py", line 151, in run
    waiter.next(waiters, actives, exc)
  File "C:\myhdlsupport\myhdl\myhdl\_Waiter.py", line 184, in next
    clauses = next(self.generator)
              ^^^^^^^^^^^^^^^^^^^^
  File "C:\myhdlsupport\myhdl\myhdl\_always_comb.py", line 79, in genfunc
    func()
  File "C:\myhdlsupport\workspace-MyHDL-support\issues\mbuesch\crc_10_10.py", line 55, in logic
    crcOut[0].next = crcIn[5] ^ crcIn[7] ^ crcIn[8] ^ crcIn[9] ^ data[5] ^ data[7] ^ data[8] ^ data[9]
    ^^^^^^^^^^^^^^
AttributeError: 'bool' object has no attribute 'next'

Indexing a Signal(intbv(0)[w:]) returns the bool() value of that bit and a bool() has no .next. Slicing a Signal(intbv(0)[w:]) returns an intbv() with value of that slice and an intbv() has no .next either.

About the different generated code: I have no good idea why this happens; but both are correct. IMO to be consistent with simulation (but also with the general rule that synthesis is a subset of the simulation set) , the converter should have raised an error for the originally generated code. To conclude: my proposed change is necessary to be able to simulate the generated design. And remember: MyHDL is a simulation language first (as are VHDL, Verilog and SystemVerilog).

[mbuesch]

Ok, yes. I understand. Thanks for explaining this. I never used this in simulation mode.

Do you want to send a PR?