Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature consideration? #263

Open
KelvinChung2000 opened this issue Dec 7, 2023 · 22 comments
Open

Feature consideration? #263

KelvinChung2000 opened this issue Dec 7, 2023 · 22 comments
Labels
enhancement

Comments

@KelvinChung2000
Copy link

After exploring and playing with pymtl3, I can feel how this can speed up hardware development. I have some suggestions for features that might enhance the experience of using pymtl3.

Better slicing
Allow the Bits slice to work like Python slicing. For example allow, a[start:] or a[:end] for accessing a[start:nBits] or a[0:end].

Singed bit
Allow Bits to be signed or provide a $signed like Verilog does. This is just something nice to have out of the box instead of me needing to write it when simulating and testing with negative values.

Making more function and class static
For example, things like b1 or b32 are statically available, which allows for static analysis from IDE and hinting. Things added in simulation pass like sim_tick, letting those also be statically available. Just seeing what function is available by the IDE static analysis saves a lot of time in finding relevant information. Another benefit is you can document each of the individual functions within the code, and the IDE will display them, which results in less documentation needing to be maintained. A good example is Networkx. They have the best open-source documentation I have ever seen.

Introducing match
Starting from Python3.10, the match statement is available, and it would be nice if we could use them for constructing hardware similar to how Verilog have a case statement.

Variable naming control
Some constant variables will be named __const__something or __tmpvar__something. It would be nice if passes were available for renaming them.

Loop unrolling
Including passes that unroll for loop statements. This might defeat the purpose of generating readable Verilog. Sometimes, reading the statement as it is rolled is more difficult, but easier to construct with a loop.

Once again, thanks for the great work.
@ptpan
Copy link
Contributor

ptpan commented Dec 7, 2023

Thanks for these comments! I think many of your proposals are very related to the translation mechanism of PyMTL. Slicing, match statements, and variable naming are feasible with the current implementation of translators. Translation support for loop unrolling would be tricky though, especially if you are referring to loops in those behavioral @update blocks. Currently the translator performs a source-to-source translation from PyMTL DSL to Verilog, and unrolling loops in update blocks could be challenging in this implementation.

I think a $signed free helper function should not be too hard? As for making classes/functions static, I'm wondering if having a type annotation stub to the pymtl3 module could help. Python typeshed has been pretty good at providing hints for static analysis, and having something similar for pymtl doesn't sound bad at all?

@cbatten
Copy link
Contributor

cbatten commented Dec 7, 2023

@KelvinChung2000 Excellent suggestions! We are very much interested in growing the base of PyMTL3 developers. If you are interested in maybe taking a stab at one of these and creating a pull request we would be happy to work with you to get it merged in ... I think supporting the extra slicing syntax would be a relatively straight-forward place to start? @ptpan do you agree? If so, and if @KelvinChung2000 you are interested let us know and we can point you to where in the code to maybe take a stab add support for the extra slicing syntax ...

@KelvinChung2000
Copy link
Author

I am happy to do some development when I have time.

@KelvinChung2000
Copy link
Author

Hi, I would like to know if there is any current implementation of the VcdGenerationPass that will separate bitstruct into individual signals?

@cbatten
Copy link
Contributor

cbatten commented Mar 25, 2024

We don't currently have such an implementation ... would definitely be useful though ... not sure how much work it would be? @jsn1993 any thoughts?

@KelvinChung2000
Copy link
Author

If you can point me to how to tell if a type is a bitstruct type, I might be able to do something.

@yo96
Copy link
Contributor

yo96 commented Mar 26, 2024 via email

@cbatten
Copy link
Contributor

cbatten commented Mar 26, 2024

I think this is where bitstructs get flattened?

pymtl3/pymtl3/passes/tracing/VcdGenerationPass.py

Lines 237 to 244 in 4b3bc18

# If we encounter a BitStruct then dump it as a concatenation of
# all fields.
# TODO: treat each field in a BitStruct as a separate signal?
try:
net_bits_bin = eval(repr(signal)).to_bits()
except Exception as e:
raise TypeError(f'{e}\n - {signal} becomes another type. Please check your code.')

So we would need to do an explicit check first to see if signal is a bitstruct. So first thing would be to create a very simple minimal example and then maybe use this:

pymtl3/pymtl3/datatypes/bitstructs.py

Lines 78 to 80 in 4b3bc18

def is_bitstruct_inst( obj ):
"""Returns True if obj is an instance of a dataclass."""
return hasattr(type(obj), _FIELDS)

To print out if the signal is a bitstruct ... if that works then we would need to recurse into the fields ... but just detecting bitstructs during VCD gen might be a first step? I gave this a shot. First, I a created the following simple test case:

from pymtl3 import *

@bitstruct
class Pair:
  a : Bits8
  b : Bits8

class BitStructPassThru( Component ):

  def construct( s ):

    s.in_   = InPort ( Pair )
    s.out   = OutPort( Pair )

    s.in_a  = InPort ( Bits8 )
    s.in_b  = InPort ( Bits8 )
    s.out_a = OutPort( Bits8 )
    s.out_b = OutPort( Bits8 )

    s.out   //= s.in_
    s.out_a //= s.in_a
    s.out_b //= s.in_b

  def line_trace( s ):
    return f"{s.in_}|{s.in_a}|{s.in_a} () {s.out}|{s.out_a}|{s.out_b}"

top = BitStructPassThru()
top.apply( DefaultPassGroup(linetrace=True,vcdwave="pymtl3_bitstruct_vcd_test") )

top.sim_reset()
for input_value in [ (0x00,0x00), (0x01,0x10), (0xa0,0xb0), (0xab,0xbc) ]:
  top.in_  @= Pair(input_value[0],input_value[1])
  top.in_a @= input_value[0]
  top.in_b @= input_value[1]
  top.sim_tick()

top.sim_tick()
top.sim_tick()
top.sim_tick()

Then after hacking around a bit I ended up adding this to the VcdGenerationPass.py:

  ...
      for i, (signal, symbol) in enumerate( net_details ):

        # If we encounter a BitStruct then dump it as a concatenation of
        # all fields.
        # TODO: treat each field in a BitStruct as a separate signal?

        if is_bitstruct_class(signal.get_type()):
          print(f"{signal} is of type {signal.get_type()} which is a bitstruct")
          for field_name in eval(repr(signal)).__bitstruct_fields__.keys():
            full_field_name = f"{repr(signal)}.{field_name}"
            print(f"  field {field_name} value is {eval(full_field_name)}")

        try:
          net_bits_bin = eval(repr(signal)).to_bits()
        except Exception as e:
          raise TypeError(f'{e}\n - {signal} becomes another type. Please check your code.')
  ...

As @yo96 mentioned I needed to import is_bitstruct_class like this:

from pymtl3.datatypes.bitstructs import is_bitstruct_class

Running my simple example produces this:

% python pymtl3_bitstruct_vcd_test.py
...
s.in_ is of type <class '__main__.Pair'> which is a bitstruct
  field a value is 01
  field b value is 10
  5: a0:b0|a0|a0 () a0:b0|a0|b0
s.in_ is of type <class '__main__.Pair'> which is a bitstruct
  field a value is a0
  field b value is b0
  6: ab:bc|ab|ab () ab:bc|ab|bc
...

So that is at least a starting point? We would need to update where the header is written to the VCD file and also make sure to handle the default values too on cycle 0. I would get everything working with only simple fields (i.e., all fields are Bits, no fields that are lists or BitStructs).

@yo96 @ptpan @jsn1993 do you think the approach I sketch above is the right way?

Would be awesome to have some more PyMTL hackers though so if you are interested maybe we can work together to add this feature in a PR?

@yo96
Copy link
Contributor

yo96 commented Mar 26, 2024 via email

@KelvinChung2000
Copy link
Author

I am thinking of doing something similar. I will implement it if I reach a point where I cannot progress without seeing each of the struct fields. For now, I can still get away with putting the fields in a different order.

I have considered moving to a more "traditional" flow, or eventually, I will need to, but I don't want to spend time learning new things if I can do everything well enough in a single language.

@yo96
Copy link
Contributor

yo96 commented Mar 27, 2024 via email

@KelvinChung2000
Copy link
Author

I don't know this is available at all. Thanks for the information.

@KelvinChung2000
Copy link
Author

My design is failing with the following assert

        # NOTE: this assertion can fail due to connections that
        # are made outside the component that has them. so i'm removing
        # this for now until we can figure out a better way to do sanity
        # check here.
        # assert len(ordered_conns) == len(m_conns_set)

        for i, x in enumerate(ordered_conns):
            if x not in m_conns_set:
                x = (x[1], x[0])
                print(x)
                assert x in m_conns_set, (
                    "There is a connection missing from "
                    "connect_order. Please contact PyMTL developers!"
                )
                ordered_conns[i] = x

The connection that causes the problem is this (s.tile[0][0].inputCrossbar.in_[5].rdy, s.tile[0][0].inputCrossbar.in_[5].en)

@cbatten
Copy link
Contributor

cbatten commented Mar 27, 2024

Can you show what the line is that is causing the assertion? Is it something like this:

  connect( s.tile[0][0].inputCrossbar.in_[5].rdy, s.tile[0][0].inputCrossbar.in_[5].en )

Is the tile a component and the inputCrossbar is a component inside the tile? If so, then this is a hierarchical structural connection. It is not translatable ... if you were to do something similar in Verilog it also is not synthesizable. It does not model real hardware ... for structural connections to be translatable/synthesizable they need to be contained within one level of the hierarchy ... if a tile contains a crossbar and you want to connect some ports on that crossbar then the tile has to be the one to make those connections not the parent of the tile ... but maybe I am missing something?

@KelvinChung2000
Copy link
Author

This is the connection s.inputCrossbar.in_[5].en //= s.inputCrossbar.in_[5].rdy

@cbatten
Copy link
Contributor

cbatten commented Mar 27, 2024

Ah ... ok ... that is not a hierarchical reference so ignore what I said above .... looks ok to me ... we would need to see a minimal failing example or at least your code to help debug ... I am assuming in_ is some kind of PyMTL interface?

@KelvinChung2000
Copy link
Author

in_ is the following interface. This is the same as the one used in CGRA-Flow.

class RecvIfcRTL( CalleeIfcRTL ):

  def construct( s, Type ):
    super().construct( en=True, rdy=True, MsgType=Type, RetType=None )

  def connect( s, other, parent ):

    # We are doing SendCL (other) -> [ RecvCL -> SendRTL ] -> RecvRTL (s)
    # SendCL is a caller interface
    if isinstance( other, CallerIfcCL ):
      m = RecvCL2SendRTL( s.MsgType )

      if hasattr( parent, "RecvCL2SendRTL_count" ):
        count = parent.RecvCL2SendRTL_count
        setattr( parent, "RecvCL2SendRTL_" + str( count ), m )
      else:
        parent.RecvCL2SendRTL_count = 0
        parent.RecvCL2SendRTL_0 = m

      connect_pairs(
        other,  m.recv,
        m.send.msg, s.msg,
        m.send.en,  s.en,
        m.send.rdy, s.rdy
      )
      parent.RecvCL2SendRTL_count += 1
      return True

    elif isinstance( other, CalleeIfcCL ):
      if s._dsl.level <= other._dsl.level:
        raise InvalidConnectionError(
            "CL2RTL connection is not supported between RecvIfcRTL"
            " and CalleeIfcCL.\n"
            "          - level {}: {} (class {})\n"
            "          - level {}: {} (class {})".format(
                s._dsl.level, repr( s ), type( s ), other._dsl.level,
                repr( other ), type( other ) ) )

      m = RecvCL2SendRTL( s.MsgType )

      if hasattr( parent, "RecvCL2SendRTL_count" ):
        count = parent.RecvCL2SendRTL_count
        setattr( parent, "RecvCL2SendRTL_" + str( count ), m )
      else:
        parent.RecvCL2SendRTL_count = 0
        parent.RecvCL2SendRTL_0 = m

      connect_pairs(
        other,  m.recv,
        m.send.msg, s.msg,
        m.send.en,  s.en,
        m.send.rdy, s.rdy
      )
      parent.RecvCL2SendRTL_count += 1
      return True

    return False

@cbatten
Copy link
Contributor

cbatten commented Mar 27, 2024

Oh man .. not sure about that code at all ... FYI, at least in my group, we have moved away from en/rdy and back to val/rdy ... I wonder if part of the issue is you are directly connecting the output rdy signal of an interface to the input en signal of that same interface ... that seems a little strange? And we also moved away from this CL stuff when just doing RTL modeling ... we now just use pure RTL everywhere when doing RTL modeling (including pure RTL sources/sinks for testing) .. simplifies things ... we could maybe take a closer look but you would have to push your code to github and provide step-by-step instructions on how to reproduce the error ... the smaller the failing test case the better :)

@KelvinChung2000
Copy link
Author

In the current mast, under pymtl3/stdlib/ifcs/send_recv_ifcs.py, it is still using en/rdy...

I am connecting it this way because I want to invalidate the output when I receive backpressure from the pipeline.

This is the smallest example I can produce

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


class RecvIfcRTL(CalleeIfcRTL):
    def construct(s, Type):
        super().construct(en=True, rdy=True, MsgType=Type, RetType=None)


class testInner(Component):
    def construct(s):
        s.in_ = RecvIfcRTL(mk_bits(2))

        @update
        def upblk():
            s.in_.rdy @= 1


class testOuter(Component):
    def construct(s):
        s.inner = testInner()
        s.inner.in_.en //= s.inner.in_.rdy


if __name__ == "__main__":
    m = testOuter()
    m.elaborate()
    m.set_metadata(VerilogTranslationPass.enable, True)
    m.apply(VerilogTranslationPass())

@yo96
Copy link
Contributor

yo96 commented Mar 27, 2024 •
edited
Loading

I think that is because of a limitation of the translation pass. I remember running into this issue a few years ago as well. A workaround is to change the structural connection to a behavioral update block like the following:

        @update
        def up_blk():
          s.inner.in_.en @= s.inner.in_.rdy

Alternatively, you can create an intermediate signal like this:

        s.inner_in_en = Wire()
        s.inner.in_.en  //= s.inner_in_en
        s.inner.in_.rdy //= s.inner_in_en

Basically it confuses the translation pass when you are directly connecting the ports of the same component outside the component. I can't recall the exact details at the moment, but @ptpan is the expert of the translation pass and can provide a more in-depth explanation.

I hope these workarounds help you resolve the issue and get your code up and running smoothly!

@cbatten
Copy link
Contributor

cbatten commented Mar 27, 2024

At least internally I have started using the pymtl4.0-dev branch. The whole send/recv thing with en/rdy was a bit of an experiment ... but ultimately I ended up going back to val/rdy for streaming interfaces ...

I just tried your example and was able to reproduce the issue ... if you use this:

class testOuter(Component):
    def construct(s):
        s.inner = testInner()
        s.temp = Wire(1)
        s.inner.in_.en  //= s.temp
        s.inner.in_.rdy //= s.temp

It goes away and the translation works fine ... I am not quite sure why this fixes things to be honest? ... looks like @yo96 was helping debug at the same time!

@KelvinChung2000
Copy link
Author

Thanks for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cbatten @yo96 @ptpan @KelvinChung2000