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

Consider slice as signal in Record initialization #402

Closed
andresdemski opened this issue Jun 9, 2020 · 5 comments
Closed

Consider slice as signal in Record initialization #402

andresdemski opened this issue Jun 9, 2020 · 5 comments
Labels

Comments

@andresdemski
Copy link

andresdemski commented Jun 9, 2020

Hi,

I'm trying to generate a Record from sliced signals (i'm trying to resize a bus) and record fields must be records or signals.

Example code:

a = Signal(32)                                                                                      
b = Signal()                                                                                        
rec = Record([('a', 5), ('b', 1)], fields={'a': a[:5], 'b': b})                                     

Error:

AssertionError                            Traceback (most recent call last)
<ipython-input-278-9d7bc72bde0a> in <module>
----> 1 rec = Record([('a', 5), ('b', 1)], fields={'a': a[:5], 'b': b})

~/.local/lib/python3.8/site-packages/nmigen/hdl/rec.py in __init__(self, layout, name, fields, src_loc_at)
    124                     assert isinstance(field, Record) and field_shape == field.layout
    125                 else:
--> 126                     assert isinstance(field, Signal) and field_shape == field.shape()
    127                 self.fields[field_name] = field
    128             else:

Maybe changing ithe following line is enough to support this feature

diff --git a/nmigen/hdl/rec.py b/nmigen/hdl/rec.py
index be89342..dd21464 100644
--- a/nmigen/hdl/rec.py
+++ b/nmigen/hdl/rec.py
@@ -134,7 +134,7 @@ class Record(UserValue):
                 if isinstance(field_shape, Layout):
                     assert isinstance(field, Record) and field_shape == field.layout
                 else:
-                    assert isinstance(field, Signal) and field_shape == field.shape()
+                    assert isinstance(field, Value) and field_shape == field.shape()
                 self.fields[field_name] = field
             else:
                 if isinstance(field_shape, Layout):
~

Thank you.

@whitequark
Copy link
Member

whitequark commented Jun 9, 2020

That patch wouldn't work; even disregarding any language design concerns, a Record cannot consist of arbitrary Values because its parts have to be lvalues (i.e. assignable).

More to your point, can you describe in detail what are you trying to do? It is not clear to me that your solution is a good one for the problem you're experiencing.

@andresdemski
Copy link
Author

andresdemski commented Jun 9, 2020

I'm trying to resize an AXI bus:

def resize_axi(axi, data_w=None, addr_w=None, id_w=None):
    layout = []
    fields = {}
    for field in axi.fields:
        width = len(axi[field])
        if data_w is not None and 'DATA' in field:
            width = data_w
        if addr_w is not None and 'ADDR' in field:
            width = addr_w
        if id_w is not None and 'ID' in field and 'VALID' not in field:
            width = id_w
        layout.append((field, width))
        fields[field] = axi[field][:width]
    return Record(layout, fields=fields, name=axi.name + '_resized')

Is there any other way to do that without comb assignments? I haven't the module object in the scope.

@whitequark
Copy link
Member

whitequark commented Jun 9, 2020

Ah right so this is actually indicative of a fairly serious problem in nMigen, which is that our unit of abstraction is an Elaboratable and it's pretty heavyweight. I think we should fix that rather than adding workarounds to Records, which already struggle to serve all the use cases people want them for.

For now I suggest you pass the module object around; I'm aware that what you're trying to do is currently pretty painful, I don't have a good solution yet, but it's something I consider important to fix in a nice way.

@andresdemski
Copy link
Author

andresdemski commented Jun 9, 2020

Thank you for your answer. Let me know if i can help.

For solving this i'm going to create an AxiResizer elaboratable. I think that it is better than passing module object.

@whitequark
Copy link
Member

whitequark commented Jun 9, 2020

Thank you for your answer. Let me know if i can help.

Not yet, there are much more pressing design issues. We'll get to it eventually.

For solving this i'm going to create an AxiResizer elaboratable. I think that it is better than passing module object.

Also an excellent solution if you are OK with the overhead of an elaboratable. That's what nmigen-soc currently does.

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

No branches or pull requests

2 participants