amaranth-lang / amaranth

A modern hardware definition language and toolchain based on Python
https://amaranth-lang.org/docs/amaranth/
BSD 2-Clause "Simplified" License
1.54k stars 170 forks source link

`Struct` inside `Record` with `DIR_FANIN` causes TypeError #800

Closed hannesha closed 1 year ago

hannesha commented 1 year ago

Since RFC 15, using aggregate types inside Record is possible.

Consider the following code:

from amaranth import *
from amaranth.lib import data

from amaranth.hdl.rec import Record, DIR_FANIN, DIR_FANOUT

class Frame(data.Struct):
    payload: unsigned(8)
    header: unsigned(8)

    # def __or__(self, other):
    #   if not isinstance(other, type(self)):
    #       raise NotImplementedError(f'Type mismatch: {type(self)}, {type(other)}')

    #   return type(self)(self.as_value() | other.as_value())

class TxBus(Record):
    def __init__(self):
        super().__init__([
            ('frame', Frame, DIR_FANIN),
            # ('frame', Frame.as_shape(), DIR_FANIN), # test with StructLayout
            ('strobe', 1, DIR_FANIN),
            ('ready', 1, DIR_FANOUT),
        ])

m = Module()

tx_sink = TxBus()
tx_source = TxBus()
tx_source2 = TxBus()

m.d.comb += tx_sink.connect(tx_source, tx_source2)

Causes an exception in Record.connect():

  File "amaranth/hdl/rec.py", line 250, in connect
    stmts += [item.eq(reduce(lambda a, b: a | b, subord_items))]
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "amaranth/hdl/rec.py", line 250, in <lambda>
    stmts += [item.eq(reduce(lambda a, b: a | b, subord_items))]
                                          ~~^~~
TypeError: unsupported operand type(s) for |: 'header' and 'header'

When using a Layout in the Record constructor yields the following error instead:

TypeError: unsupported operand type(s) for |: 'View' and 'View'

Defining the or operator with __or__ works for structs as a workaround. However it isn't possible to do this for Layouts.

Another possibility is to get the underlying shape with .as_shape() and access the field with views. This kind of defeats the convenience of using a struct though.

whitequark commented 1 year ago

Records are going to be deprecated and removed on a very aggressive schedule so I don't see this as being worth fixing.

hannesha commented 1 year ago

Is there going to be a Interface replacement for Record before its removal? Modeling Interfaces is the only remaining use-case of Record since the introduction of aggregate types.

whitequark commented 1 year ago

Yes, Records will be removed simultaneously with introducing a replacement, and they will coexist for one release cycle. (This is how Amaranth always does removal of anything that's been in a release.)

hannesha commented 1 year ago

Great, Thanks. I'll try out the replacement once it's implemented.

Should I keep this issue open?

whitequark commented 1 year ago

I think we can close this as not planned.