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

Add support for magma.Tuple to be a top level port #241

Open
rsetaluri opened this issue Aug 7, 2018 · 11 comments
Open

Add support for magma.Tuple to be a top level port #241

rsetaluri opened this issue Aug 7, 2018 · 11 comments

Comments

@rsetaluri
Copy link
Collaborator

import magma as m


class Foo(m.Circuit):
    IO = ["IFC", m.Tuple(I=m.In(m.Bit), O=m.Out(m.Bit))]

    @classmethod
    def definition(io):
        pass


if __name__ == "__main__":
    m.compile("foo", Foo, output="coreir")
    print (open("foo.json").read())

The above fails in the compile() call. The same holds for output=verilog but that's simply because magma.Tuple is simply not allowed in a verilog module declaration.

@rsetaluri
Copy link
Collaborator Author

The underlying issue seems to be that magma top level ports need to have isinput() or isoutput() == True in the core ir backend.

@phanrahan
Copy link
Owner

Currently magma modules can only have type Bit or Bits. Tuples or more general arrays are not supported.

@leonardt
Copy link
Collaborator

leonardt commented Aug 7, 2018

We should be able to support this in the coreir backend. The verilog backend doesn't have support for basic Tuples yet, so this definitely won't work for that.

@rsetaluri
Copy link
Collaborator Author

import magma as m


class Foo(m.Circuit):
    IO = ["I", m.In(m.Tuple(a=m.Bit, b=m.Bit)), "O", m.Out(m.Tuple(a=m.Bit, b=m.Bit))]

    @classmethod
    def definition(io):
        pass


if __name__ == "__main__":
    m.compile("foo", Foo, output="coreir")
    print (open("foo.json").read())

To be clear the above does work because the top level types are In/Out.

@rsetaluri
Copy link
Collaborator Author

@phanrahan: Is this true? I think Tuples and Arrays seem to be mostly working for the CoreIR backend. If it is the case, what is the purpose of these types? Just for internal use?

@leonardt: understood.

@phanrahan
Copy link
Owner

The intention is to support all types in module interfaces.

@rsetaluri rsetaluri reopened this Aug 7, 2018
@leonardt
Copy link
Collaborator

leonardt commented Aug 7, 2018

Support for more complex arrays (e.g. nested) and tuples in the verilog backend require flattening the types to normal verilog types (1-D arrays, wire). We haven't added that yet since we wanted to be careful about how to do this right since it requires munging names. Coreir has built-in support for those types so we can support it, and the corier-verilog backend has implemented name munging for generating those types.

@rsetaluri
Copy link
Collaborator Author

To provide some more context:

import magma as m

foo = m.DefineCircuit("Foo", "ifc", m.Tuple(I=m.In(m.Bit), O=m.Out(m.Bit)))
m.wire(foo.ifc.I, foo.ifc.O)
m.EndCircuit()

m.compile("foo", foo, output="coreir")

This errors in compilation. I gave a shot at adding support for this but I noticed several things that needed to change (including that #260 was causing oddities in directionality in interface vs IO).

The expected CoreIR is:

{"top":"global.Foo",
"namespaces":{
  "global":{
    "modules":{
      "Foo":{
        "type":["Record",[
          ["ifc",["Record",[["I","BitIn"],["O","Bit"]]]]
        ]],
        "connections":[
          ["self.ifc.I", "self.ifc.O"]
        ]
      }
    }
  }
}
}

@rsetaluri
Copy link
Collaborator Author

Noticed something that doesn't work:

>>> T = Array(10, Tuple(I=In(Bit), O=Out(Bit)))
>>> Foo = magma.DefineCircuit("Foo", "IFC", T)
>>> for i in range(10):
...     wire(Foo.IFC[i].I, Foo.IFC[i].O)
... 
>>> EndCircuit()
>>> compile("foo", Foo, output="coreir")
>>>
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/setaluri/dev/magma/magma/compile.py", line 110, in compile
    __compile_to_coreir(main, file_name, opts)
  File "/Users/setaluri/dev/magma/magma/compile.py", line 66, in __compile_to_coreir
    backend.compile(main)
  File "/Users/setaluri/dev/magma/magma/backend/coreir_.py", line 400, in compile
    self.modules[defn.name] = self.compile_definition(defn)
  File "/Users/setaluri/dev/magma/magma/backend/coreir_.py", line 291, in compile_definition
    module_type = self.convert_interface_to_module_type(definition.interface)
  File "/Users/setaluri/dev/magma/magma/backend/coreir_.py", line 183, in convert_interface_to_module_type
    raise NotImplementedError()
NotImplementedError

Looking at the code at line 183:

def convert_interface_to_module_type(self, interface):
args = OrderedDict()
for name, port in interface.ports.items():
if not port.isinput() and not port.isoutput() and \
not isinstance(port, TupleType):
raise NotImplementedError()
args[name] = self.get_type(port, port.isinput())
return self.context.Record(args)

We always assert that a type is an In or an Out when converting interfaces -- unless the type is Tuple. With nesting however, this breaks down. Not sure what the right thing to do here is, because we want to make sure that everything has a direction at it's base level (e.g. all members of a tuple). Perhaps something like this?

def is_directed(T):
    if isinstance(T, magma.BitKind):
        return T.isinput() or T.isoutput()
    if isinstance(T, magma.ArrayKind):
        return all([is_directed(t) for t in T.Ts])
    if isinstance(T, magma.TupleKind):
        return all([is_directed(t) for t in T.Ts])
    raise NotImplementedError(T)

@leonardt
Copy link
Collaborator

leonardt commented Sep 6, 2018

Fixed by #274, basically the logic to handle mixed direction children was already there, just only used for Tuples. Anytime that it checked if it was a tuple that could have mixed direction children, it now checks if it's an array that could have mixed direction children.

@phanrahan
Copy link
Owner

Looking at the source, I would think this should work:

def is_directed(T):
    return T.isinput() or T.isoutput()

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