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.

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

Unable to use concat with InPort #279

Closed
KelvinChung2000 opened this issue Jun 3, 2024 · 19 comments
Closed

Unable to use concat with InPort #279

KelvinChung2000 opened this issue Jun 3, 2024 · 19 comments

Comments

@KelvinChung2000
Copy link

The following example,

from pymtl3 import *


class MasterIfc(Interface):
    def construct(s):
        s.in_ = InPort(b2)


class Test(Component):
    def construct(s):
        s.ins_ = [MasterIfc() for _ in range(4)]
        s.out = OutPort(b8)
        s.out //= concat(s.ins_[0].in_, s.ins_[1].in_, s.ins_[2].in_, s.ins_[3].in_)


if __name__ == "__main__":
    from pymtl3.passes.backends.verilog import VerilogTranslationPass

    m = Test()
    m.elaborate()
    m.set_metadata(VerilogTranslationPass.enable, True)
    m.set_metadata(VerilogTranslationPass.explicit_file_name, "Test.sv")
    m.apply(VerilogTranslationPass())
    m.apply(DefaultPassGroup())

is giving me

  File "/home/kelvin/pymtl3/pymtl3/dsl/Connectable.py", line 184, in __getattr__
    raise AttributeError("{} is not a signals with struct type, and has no attribute '{}'".format( s, name ))
AttributeError: s.ins_[0].in_ is not a signals with struct type, and has no attribute 'nbits'

Is there any way I can work around this?

@cbatten
Copy link
Contributor

cbatten commented Jun 3, 2024

How does this relate to your recent pull request?

I am not so sure about this general trend ... I think in general concat is meant for use inside an update block right? Does it work with //= out of the box? This seems like a slippery slope ... should we also make zext and sext work with //= ... we did have support for a lambda with //= like this:

  s.out //= lambda: concat( s.a, s.b )

But in retrospect I think it is just not worth it. I kind of think we should deprecate lambda's with //= ... indeed I think we should maybe just deprecate //= completely and just rely on connect ... thoughts?

@cbatten
Copy link
Contributor

cbatten commented Jun 3, 2024

@ptpan @yo96 @jsn1993 thoughts?

@KelvinChung2000
Copy link
Author

KelvinChung2000 commented Jun 3, 2024

I want to achieve something like the following:

class MasterIfc(Interface):
    def construct(s):
        s.in_ = InPort(b2)


class Test(Component):
    def construct(s, n):
        s.ins_ = [MasterIfc() for _ in range(n)]
        s.out = OutPort(b8)
        s.out //= concat(i.in_ for i in s.ins_)

I have tried to use s.out //= lambda: concat( i.in_ for i in s.ins_ ) but this will causes the following error with my pull request

  File "/home/kelvin/pymtl3/pymtl3/passes/rtlir/behavioral/BehavioralRTLIRGenL1Pass.py", line 503, in visit_GeneratorExp
    raise PyMTLSyntaxError( s.blk, node, 'invalid operation: generator expression' )
pymtl3.passes.rtlir.errors.PyMTLSyntaxError: 
In file /home/kelvin/fullSystemDesign/_lambda__s_out, Line 1, Col 24:
  def _lambda__s_out():

I would say doing s.out //= lambda: concat( s.a, s.b ) is equivalent to assign out = {a, b};

@cbatten
Copy link
Contributor

cbatten commented Jun 3, 2024

I think there are many different questions/issues here.

First, is does concat work with //= normally ... does this work?

s.out //= concat( s.a, s.b )

I am actually not sure ...

Second, is should we support list comprehensions inside a concat. I think these are orthogonal issues because I don't think this works either?

@update
def comb():
  s.out @= concat( i.in for i in s.ins_ )

Third, what does it mean to concat interfaces? I am not sure that is reasonable ... interfaces can and do include ports going in different directions so you cannot concatenate them ...

@KelvinChung2000
Copy link
Author

This works (with my pull request):

 s.out //= concat(b2(2) for i in range(4))

This does not:

s.ins2_ = InPort(b2)
s.out //= concat(s.ins2_ for i in range(4))

I think concat on an interface does not make sense since it is difficult to tell how concat aligns the interface. However, I think contacting signals within the interface makes sense, as it is common to pick out specific signals within the interface to do specific operations.

@yo96
Copy link
Contributor

yo96 commented Jun 3, 2024

How does this relate to your recent pull request?

I am not so sure about this general trend ... I think in general concat is meant for use inside an update block right? Does it work with //= out of the box? This seems like a slippery slope ... should we also make zext and sext work with //= ... we did have support for a lambda with //= like this:

  s.out //= lambda: concat( s.a, s.b )

But in retrospect I think it is just not worth it. I kind of think we should deprecate lambda's with //= ... indeed I think we should maybe just deprecate //= completely and just rely on connect ... thoughts?

Lambda with //= corresponds to the assign state in Verilog. As I remember it was very challenging to support connecting to an arbitrary statement without the lambda.

@KelvinChung2000
Copy link
Author

Is the limitation caused by an older version of Python or something else?

@cbatten
Copy link
Contributor

cbatten commented Jun 3, 2024

It is really just a constraint of the underlying framework ... //= was always supposed to be syntactic sugar for connect ... but then we figured out how use a lambda to create an update block on the fly which seemed cool ... but in retrospect it just adds complexity .... so I really do not think we can support this syntax:

  s.out //= concat( s.a, s.b )

@KelvinChung2000
Copy link
Author

However with

class MasterIfc(Interface):
    def construct(s):
        s.in_ = InPort(b2)


class Test(Component):
    def construct(s):
        s.ins_ = [MasterIfc() for _ in range(4)]
        s.out = OutPort(b8)
        connect(
            s.out, concat(s.ins_[0].in_, s.ins_[1].in_, s.ins_[2].in_, s.ins_[3].in_)
        )

It is still not working at the moment and yielding the following error:

  File "/home/kelvin/pymtl3/pymtl3/dsl/Connectable.py", line 184, in __getattr__
    raise AttributeError("{} is not a signals with struct type, and has no attribute '{}'".format( s, name ))
AttributeError: s.ins_[0].in_ is not a signals with struct type, and has no attribute 'nbits'

I like the //= syntax as it is simple and makes sense. It shows the direction of the assignment as well, which feels similar to assign a = b;

@KelvinChung2000
Copy link
Author

Will the recent Typing system help with the problem? Pymtl will perform type-checking to force things to be evaluated based on the given type.

@cbatten
Copy link
Contributor

cbatten commented Jun 4, 2024

no ... the problem is you would have to create an update block on the fly to do the concat operation ... it would be very hard to do ...

@KelvinChung2000
Copy link
Author

If I understand correctly, the on-the-fly update block creation might be difficult to support in a pure Python simulation environment. If that's the problem, is there a way for me to bypass this constraint and go for a verilator-based simulation or any other way such I can describe what I would like to write rather than something like the following?

class Test(Component):
    def construct(s):
        s.ins_ = [MasterIfc() for _ in range(4)]
        s.out = OutPort(b8)

        for c, i in enumerate(s.ins_):
            b = c * i.in_.get_type().nbits
            w = i.in_.get_type().nbits
            s.out[b : b + w] //= s.ins_[c].in_

@cbatten
Copy link
Contributor

cbatten commented Jun 4, 2024

Actually it is not really even the pure-Python simulation part ... the RTL translation pass does not parse //= statements ... the pass works on the elaborated model after all //= have turned into structural connections ...

Maybe you can create your own custom connect function? That is definitely doable and very much in the spirit of what you are trying to do here ... you can use a function to encapsulate this connection operation.

def connect_concat_master_ifc( out, ifcs ):
  for c, i in enumerate(ifcs):
    b = c * i.in_.get_type().nbits
    w = i.in_.get_type().nbits
    connect( out[b : b + w], ifcs[c].in_ )

maybe something like that? Not sure if that will work though ...

@KelvinChung2000
Copy link
Author

I will also need to add something to search for specific signals within the interface I want to connect. So I will need something like:

class MasterIfc(Interface):
    def construct(s):
        s.in_ = InPort(b2)
        s.in_2 = InPort(b2)


def ifcConcat(out, ifc, sig):
    for c, i in enumerate(ifc):
        ifcSig = getattr(i, sig)
        b = c * ifcSig.get_type().nbits
        w = getattr(i, sig).get_type().nbits
        connect(out[b : b + w], ifcSig)


class Test(Component):
    def construct(s):
        s.ins_ = [MasterIfc() for _ in range(4)]
        s.out = OutPort(b8)
        s.out1 = OutPort(b8)

        ifcConcat(s.out, s.ins_, "in_")
        ifcConcat(s.out1, s.ins_, "in_2")

and this will translate to:

module Test_noparam
(
  input  logic [0:0] clk ,
  output logic [7:0] out ,
  output logic [7:0] out1 ,
  input  logic [0:0] reset ,
  input logic [1:0] ins___in_ [0:3] ,
  input logic [1:0] ins___in_2 [0:3] 
);

  assign out[1:0] = ins___in_[0];
  assign out[3:2] = ins___in_[1];
  assign out[5:4] = ins___in_[2];
  assign out[7:6] = ins___in_[3];
  assign out1[1:0] = ins___in_2[0];
  assign out1[3:2] = ins___in_2[1];
  assign out1[5:4] = ins___in_2[2];
  assign out1[7:6] = ins___in_2[3];

endmodule

which works.

@cbatten
Copy link
Contributor

cbatten commented Jun 4, 2024

The power of python!

@KelvinChung2000
Copy link
Author

KelvinChung2000 commented Jun 4, 2024

It works, but I dislike how it needs to be done this way... At least I can keep working on what is important...

@cbatten
Copy link
Contributor

cbatten commented Jun 4, 2024

we are open to pull requests! if you can figure out some kind of slick way to make this cleaner please do let us know ... it just does not seem like a simple feature request. glad you are finding PyMTL3 useful!

@KelvinChung2000
Copy link
Author

I think what needs to be done is to update the concat function (or all helper functions) to operate based on signals. Then, we update the lambda update block creation logic to copy the whole body of the called functions, making the on-the-fly creation of any update block possible.

Something like:

def neg(x):
  a @= -x
  return a

b = Wire()
a //= lambda: neg(b)

should also be possible if what I am thinking works.

Other constraints will also probably be necessary, like confirming that the return value is a signal, avoiding recursive function calls, and finding ways to handle external functions.

I will need to learn how to play with the AST to extract the right information and probably a lot of small details that I am not sure about. This is probably about a week of work...

I will leave this here for future me or someone who is interested in dealing with this...

@KelvinChung2000
Copy link
Author

By the way, I noticed that some of the AST features pymtl uses will be removed in python3.14, like ast.num. So, another update might be required. Also, I noticed some comments about relative import causing circular import. I suggest changing all the imports to absolute imports, which will resolve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants