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 fully combinational syntax #225

Merged
merged 24 commits into from Aug 2, 2018
Merged

Conversation

leonardt
Copy link
Collaborator

This extends the syntax for circuit.combinational to match the proposed syntax from our planning meeting.

This implements if then else and ternary expressions inside the combinational circuit definitions. It rewrites the function into a module that can be instantiated (using the function call syntax). It defaults to having an output port named O, although we could consider adding an option parameter to the decorator that lets you set the output port name.

The major difference between this branch and the original if statement syntax proposal is that it doesn't mix wire calls with the combinational circuit syntax. This allows you to easily use the function call abstraction for combinational logic, but forces you to decompose it from structural code.

One question is, do we want to keep the ability to use if statements interleaved with structural code? That's what's proposed in #224. In this pull request, wire calls are not supported in m.circuit.combinational. It should be possible to factor it in a way to support both if we think they're both viable features.

@leonardt
Copy link
Collaborator Author

What's still missing is the ability to call other m.circuit.combinational functions inside (limited function composition, where you can't call any arbitrary python function). In theory, we could relax this constraint to be any function that accepts magma values and returns a magma value, which is what m.circuit.combinational is, but in theory someone could write there own custom function.

@coveralls
Copy link

coveralls commented Jul 29, 2018

Pull Request Test Coverage Report for Build 858

  • 74 of 80 (92.5%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 69.909%

Changes Missing Coverage Covered Lines Changed/Added Lines %
magma/circuit_def.py 50 51 98.04%
magma/conversions.py 4 5 80.0%
magma/logging.py 8 12 66.67%
Totals Coverage Status
Change from base Build 815: 0.6%
Covered Lines: 3148
Relevant Lines: 4503

💛 - Coveralls

@leonardt
Copy link
Collaborator Author

Actually turns out function composition just works if you use it correctly (you only call another m.circuit.combinational definition.

@phanrahan
Copy link
Owner

Looks great.

I think using O as the default name makes sense.

Can you return multiple values?

@leonardt
Copy link
Collaborator Author

Nope, but that's a good idea, I will add it. I propose that O becomes a m.Tuple type, that can be indexed like an array but has heterogenous elements (like a normal Python tuple). By default this wouldn't have any names associated with tuple elements, but we could allow that if they explicitly return a set of key, value pairs, e.g.

return m.tuple(x=I[0], y=I[1]))

@leonardt
Copy link
Collaborator Author

Added the scaffolding to support Tuple as a return type, but testing it is blocked by:

  • magma's verilog backend does not support tuples in the module type signature (need to "flatten" the type)
  • coreir doesn't support tuples without names (we map our named tuple type to the coreir record type). I tried mapping the tuple to record with the keys "0", "1", ..., but that's not a valid name for coreir, so either coreir needs to allow us to use "0", "1", ... as record keys, add an unnamed tuple type, or we could use the string name of the number ("one", "two").

Until then could think about merging this in and finishing support for tuples when the above two issues are resolved.

@phanrahan
Copy link
Owner

I misspoke. I meant it would be nice to generate a circuit with multiple output arguments,
not a output tuple.

For example, in mantle the counter returns O, COUT.

@leonardt
Copy link
Collaborator Author

leonardt commented Jul 30, 2018

A tuple would be the way to do that in Python (return (O, COUT)), we could implicitly flatten the tuple on the return type, but we'd need a way to automatically generate the names of the outputs, or have the user specify the name.

If they just use variable names in the return statements, we could take that, but that would prevent the user from being able to return an expression, e.g. return a + b, COUT.

Maybe we could use the custom type annotation syntax to specify the name of the output ports?

def a(I: m.In(m.Bits(2))) -> {"O": m.Bit, "COUT": m.Bit}:
    pass

or

def a(I: m.In(m.Bits(2))) -> ("O", m.Bit, "COUT", m.Bit):
    pass

The dictionary syntax might make the most sense since you're implicitly describing the return of a record/bundle type.

@leonardt
Copy link
Collaborator Author

leonardt commented Jul 30, 2018

Another option is to integrate it with the class SubClass(m.Circuit): pattern, e.g.

class Add(m.Circuit):
    IO = ["a", m.In(m.Bit), "b", m.In(m.Bit), "c", m.In(m.Bit), "O", m.Out(m.Bit), "COUT", m.Out(m.Bit)]
    @circuit.combinational
    def definition(a, b, c):
        return a ^ b ^ c, a & b | b & c | c & a

@rsetaluri
Copy link
Collaborator

Re: tuples, we could also use named tuple (https://docs.python.org/3.7/library/collections.html#collections.namedtuple).

@leonardt leonardt merged commit f55820b into if-statements Aug 2, 2018
@leonardt leonardt deleted the circuit-combinational branch August 2, 2018 01:04
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

Successfully merging this pull request may close these issues.

None yet

4 participants