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

Clarify documentation for alignment parameters to mention that it is log2 #6

Closed
jfng opened this issue Feb 5, 2020 · 7 comments
Closed
Labels

Comments

@jfng
Copy link

@jfng jfng commented Feb 5, 2020

Repro:

from nmigen.back import rtlil
from nmigen_soc import csr

csr_mux = csr.Multiplexer(addr_width=16, data_width=8, alignment=8)
csr_mux.add(csr.Element(1, "r"))

print(rtlil.convert(csr_mux))

Output:

<snip>
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in <lambda>
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in shape
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in <lambda>
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in shape
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in <lambda>
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in shape
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in <lambda>
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in shape
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 546, in <lambda>
    op_shapes = list(map(lambda x: x.shape(), self.operands))
  File "/home/jf/src/nmigen/nmigen/hdl/ast.py", line 643, in shape
    return Shape(self.stop - self.start)
  File "<string>", line 1, in __new__
RecursionError: maximum recursion depth exceeded while calling a Python object
@jfng jfng added the bug label Feb 5, 2020
@whitequark
Copy link
Member

@whitequark whitequark commented Feb 5, 2020

Could you investigate this? I'm finishing various issues in nMigen right now, and will then be busy reviewing PRs.

@jfng
Copy link
Author

@jfng jfng commented Feb 5, 2020

Sure, I'll have a look at it.

@jfng
Copy link
Author

@jfng jfng commented Feb 5, 2020

The problematic assignment is this one, which gets nested 2**alignment times:
https://github.com/nmigen/nmigen-soc/blob/3195de9f0126e736ae10e0fd360a8fbff70bc592/nmigen_soc/csr/bus.py#L256

During elaboration, _RHSValueCompiler.on_Operator_binary() needs to compute the shape of the RHS of this assignment, by recursion. However, it exceeds the maximum depth of the interpreter stack.

A more accurate repro would be:

from nmigen import *
from nmigen.back import rtlil


class Top(Elaboratable):
    def __init__(self, width):
        self.width = width

    def elaborate(self, platform):
        m = Module()

        s = Signal(self.width)
        o = Signal()

        fanin = 0
        for bit in s:
            fanin |= bit

        m.d.comb += o.eq(fanin)

        return m


if __name__ == "__main__":
    top = Top(width=256)
    print(rtlil.convert(top))

... which makes me wonder whether the bug comes from nmigen-soc or nmigen.

@whitequark
Copy link
Member

@whitequark whitequark commented Feb 6, 2020

... which makes me wonder whether the bug comes from nmigen-soc or nmigen.

Unfortunately, the bug comes from the way you are using csr.Multiplexer. It looks like I did not explain clearly how to use it. Let me try again.

First, the alignment argument is not an amount, but it is the logarithm of amount. That is, if you wanted each element to be aligned to an address that is a multiple of 8, you would specify alignment=3, because log2(8)=3. The idea here is that it removes a lot of range checks (because every positive value is valid), and also simplifies the implementation (because in many places you want log2(alignment).

Second, the alignment argument is not specified in bits, but in words. That is, if you make a CSR bus which is connected to a 8-bit host bus, and you want registers to be densely packed, you should use:

csr_mux = csr.Multiplexer(addr_width=16, data_width=8, alignment=0)

If you make a CSR bus which is connected to a 32-bit host bus, but want the internal CSR accesses to be 8-bit wide to conserve resources, and want registers to start at 32 bit boundaries, you should use:

csr_mux = csr.Multiplexer(addr_width=14, data_width=32, alignment=2)

Does this make sense?

@whitequark
Copy link
Member

@whitequark whitequark commented Feb 6, 2020

Oh, and I still consider this issue a bug, it's just a bug in documentation.

@jfng
Copy link
Author

@jfng jfng commented Feb 6, 2020

Does this make sense?

Yes, thanks for the explanation !

Oh, and I still consider this issue a bug, it's just a bug in documentation.

The MemoryMap docstring actually mentions it:

alignment : int
    Range alignment. Each added resource and window will be placed at an address that is
    a multiple of ``2 ** alignment``, and its size will be rounded up to be a multiple of
    ``2 ** alignment``.

@whitequark
Copy link
Member

@whitequark whitequark commented Feb 6, 2020

Yes; but docstrings elsewhere do not, and if you got confused, so will other people. If you feel like you can clarify these docs, I would be grateful, otherwise I'll do it later myself.

@whitequark whitequark changed the title RecursionError when elaborating a csr.Multiplexer with alignment >= 8 Clarify documentation for alignment parameters to mention that it is log2 Mar 22, 2020
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