pymtl / pymtl3

Pymtl 3 (Mamba), an open-source Python-based hardware generation, simulation, and verification framework
BSD 3-Clause "New" or "Revised" License
388 stars 46 forks source link

Inconsistent simulation #258

Closed KelvinChung2000 closed 12 months ago

KelvinChung2000 commented 1 year ago

The following script will produce different results depending on how table2Update is written.

import pymtl3 as pymtl
from pymtl3.passes.backends.verilog import *

class a(pymtl.Component):
    def construct(s):
        s.inTag = pymtl.InPort(pymtl.Bits8)
        s.add = pymtl.InPort(pymtl.Bits1)
        s.out = pymtl.OutPort(pymtl.Bits8)
        s.en = pymtl.InPort(pymtl.Bits1)
        s.inc = pymtl.InPort(pymtl.Bits1)
        numEntries = 4
        s.table = [pymtl.Wire(8) for _ in range(numEntries)]
        s.table2 = [pymtl.Wire(8) for _ in range(numEntries)]
        s.matchArray = pymtl.Wire(pymtl.mk_bits(numEntries))

        @pymtl.update
        def findMatch():
            if s.en:
                for i in range(numEntries):
                    s.matchArray[i] @= (s.table[i] == s.inTag)

        @pymtl.update_ff
        def tableUpdate():
            if s.add & (s.matchArray == 0):
                s.table[0] <<= s.inTag

        @pymtl.update_ff
        def table2Update():
            for i in range(numEntries):
                if s.matchArray[i]:
                    if s.inc & (s.table2[i] != 2**8-1):
                        s.table2[i] <<= s.table2[i] + 1
                else:
                    # incorrect update
                    # s.table2[0] <<= s.table2[i]

                    # correct update
                    s.table2[i] <<= s.table2[i]

if __name__ == "__main__":
    m = a()
    m.elaborate()
    m.apply(pymtl.DefaultPassGroup())
    m.en @= 1
    m.inc @= 1

    print("Start")
    print("match ", m.matchArray)
    print("t1 ",m.table)
    print("t2 ", m.table2)
    print()

    m.inTag @= 10
    m.add @= 1
    m.sim_tick()
    print("After Tick1")
    print("match ", m.matchArray)
    print("t1 ", m.table)
    print("t2 ", m.table2)
    print()

    m.add @= 0
    m.sim_tick()
    print("After Tick2")
    print("match ", m.matchArray)
    print("t1 ", m.table)
    print("t2 ", m.table2)
    print()

If using s.table2[0] <<= s.table2[i] will result in the following result

Start
match  0
t1  [Bits8(0x00), Bits8(0x00), Bits8(0x00), Bits8(0x00)]
t2  [Bits8(0x00), Bits8(0x00), Bits8(0x00), Bits8(0x00)]

After Tick1
match  1
t1  [Bits8(0x0a), Bits8(0x00), Bits8(0x00), Bits8(0x00)]
t2  [Bits8(0x00), Bits8(0x00), Bits8(0x00), Bits8(0x00)]

After Tick2
match  1
t1  [Bits8(0x0a), Bits8(0x00), Bits8(0x00), Bits8(0x00)]
t2  [Bits8(**0x00**), Bits8(0x00), Bits8(0x00), Bits8(0x00)]

but when using s.table2[i] <<= s.table2[i] will produce the following

Start
match  0
t1  [Bits8(0x00), Bits8(0x00), Bits8(0x00), Bits8(0x00)]
t2  [Bits8(0x00), Bits8(0x00), Bits8(0x00), Bits8(0x00)]

After Tick1
match  1
t1  [Bits8(0x0a), Bits8(0x00), Bits8(0x00), Bits8(0x00)]
t2  [Bits8(0x00), Bits8(0x00), Bits8(0x00), Bits8(0x00)]

After Tick2
match  1
t1  [Bits8(0x0a), Bits8(0x00), Bits8(0x00), Bits8(0x00)]
t2  [Bits8(**0x01**), Bits8(0x00), Bits8(0x00), Bits8(0x00)]

In both situations, the else clauses should not affect the assignment in the if clauses. However, for some reason, this is causing problems.

cbatten commented 12 months ago

I tried executing your example, but I am a little confused on what the observable error is here?

When you use s.table2[0] <<= s.table2[i] on the third cycle the value of the four match bits before the edge is 0001 ... so on the third cycle in table2Update you will end doing this:

  i = 0 : table2[0] <<= 01 (via the then clause)
  i = 1 : table2[0] <<= 00 (via the else clause)
  i = 2 : table2[0] <<= 00 (via the else clause)
  i = 3 : table2[0] <<= 00 (via the else clause)

I think this is the expected behavior as your code is written? The else clause will be executed three times for the second, third, and fourth match bits? And the else clause always sets the first entry in table2 to be zero ... so all of that looks ok?

When you use s.table2[i] <<= s.table2[i] on the third cycle the value of the four match bits will still be 0001 ... so on the third cycle in table2Update you will end doing this:

  i = 0 : table2[0] <<= 01 (via the then clause)
  i = 1 : table2[1] <<= 00 (via the else clause)
  i = 2 : table2[2] <<= 00 (via the else clause)
  i = 3 : table2[3] <<= 00 (via the else clause)

Again, the else clause will be executed three times the second, third, and fourth match bits? And the else clause sets the corresponding entry in table2 to be zero ... but table2[0] will still be 01 ... again I think everything is executing as expected given your code?

KelvinChung2000 commented 12 months ago

Ok, I understand the problem now. I am confused about the assignment order. However, in this situation, table2[0] will be having multiple drivers? Since now table2[0] can be both 0 or 1

cbatten commented 12 months ago

It is perfectly fine to write a signal multiple times in the same update block (just like in verilog). The final write takes precedence. In this case, the writes are actually disjoint which is a simpler case ... for example in Verilog:

  always @(posedge clk) begin
    if ( reset )
      out <= 0;
    else
      out <= in_;
  end

and in PyMTL3:

    @update_ff
    def upblock():
      if ( s.reset )
        s.out <<= 0;
      else
        s.out <<= s.in_;
    end

You cannot write a signal from two different update blocks (just like you cannot write a signal from two different always blocks in Verilog), but it is fine to write a signal multiple times within the same update block ... But maybe this is not your question?

KelvinChung2000 commented 12 months ago

Thanks for your clarification.

cbatten commented 12 months ago

No problem! Glad you are trying out PyMTL3!