phanrahan / magma

magma circuits
Other
243 stars 23 forks source link

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

Open rsetaluri opened 6 years ago

rsetaluri commented 6 years ago
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 commented 6 years ago

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

phanrahan commented 6 years ago

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

leonardt commented 6 years ago

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 commented 6 years ago
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 commented 6 years ago

@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 commented 6 years ago

The intention is to support all types in module interfaces.

leonardt commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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: https://github.com/phanrahan/magma/blob/f84457fc125bd54751b5c0ad5fd450255eec91c8/magma/backend/coreir_.py#L178-L185

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 commented 6 years ago

Fixed by https://github.com/phanrahan/magma/pull/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 commented 6 years ago

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

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