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

Assignment #315

Merged
merged 13 commits into from Nov 8, 2018
Merged

Assignment #315

merged 13 commits into from Nov 8, 2018

Conversation

leonardt
Copy link
Collaborator

@leonardt leonardt commented Nov 3, 2018

Note, this is based on the operators-docs branch, so should merge this first before merging that into master, or retarget this pull to master.

Adds support for the assignment operator on circuit definition ports and circuit instance ports. Here's an example snippet from one of the tests

    circ = m.DefineCircuit(name, "a", m.In(T), "b", m.In(T),
                           "c", m.Out(T))
    and2 = DeclareAnd(width)()
    and2.I0 = circ.a
    and2.I1 = circ.b
    circ.c = and2.O
    m.EndDefine()

It will raise a typeerror if you try to assign to an Input of a definition or an output of an instance.

One issue is that we're overloading the meaning of assignment here (specifically __setitem__), which is a foreign pattern for Python users. That is, magma_obj.i = v no longer means, set the field i to v on magma_obj if i is an input port, instead it means wire v to i. An alternative syntax that should consider is using something like <=. This has the benefit of being familiar to verilog programmers (non-blocking assign), and introduces less confusion with the = semantics. That is, it would preserve the mental model that assigning to a field on an object just performs a __setattr__.

The use of <= without assigning the result to a value is probably not a common Python pattern (if used at all), so at least we're not overloading some expected behavior which might lead to trouble, particularly when onboarding a new user coming from Python.

Here's what the alternative syntax would look like

    circ = m.DefineCircuit(name, "a", m.In(T), "b", m.In(T),
                           "c", m.Out(T))
    and2 = DeclareAnd(width)()
    and2.I0 <= circ.a
    and2.I1 <= circ.b
    circ.c <= and2.O
    m.EndDefine()

Thoughts? After some thinking, my current inclination is towards using <= for the above mentioned reason related to breaking Python's standard semantics of =.

@leonardt
Copy link
Collaborator Author

leonardt commented Nov 3, 2018

Related issue from Mantle phanrahan/mantle#90

@coveralls
Copy link

coveralls commented Nov 3, 2018

Pull Request Test Coverage Report for Build 1260

  • 11 of 12 (91.67%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 72.068%

Changes Missing Coverage Covered Lines Changed/Added Lines %
magma/util.py 6 7 85.71%
Totals Coverage Status
Change from base Build 1258: 0.05%
Covered Lines: 3496
Relevant Lines: 4851

💛 - Coveralls

@leonardt
Copy link
Collaborator Author

leonardt commented Nov 3, 2018

Actually, using <= would allow us to move the logic into the magma value object, instead of the circuit definition/instance object.

Then, code like this would work

O = circ.O
O <= ~circ.I

Basically, to use =, we need to override __setattr__ on an object, so we can't overload = for a standalone Python variable (even a reference to the attribute we're trying to set). Using <= would dispatch to __lte__ on the magma value, so this could be used for variables that reference the value. Another reason to go with <= instead of =.

@phanrahan
Copy link
Owner

Nice.

I think overloading the value object is better. It is more general. It also preserves the semantics of =.

I don't think there is any downside, assuming that the lhs is an inputs. The <= should still correspond to greater than or equal if the lhs and the rhs are both outputs.

One slightly ambigious case is when the lhs is neither an input or an output. For example, an anonymous value which acts like a wire. Probably makes sense to treat that case as a wiring operator, but I haven't thought that through.

@leonardt leonardt changed the base branch from operator-docs to master November 8, 2018 19:41
@phanrahan
Copy link
Owner

One thing to watch out for using <=

for i in range(N):
    I[i] <= main.J1[i] if i < 8 else 0

doesn't work since <= has lower precedence than x if p then y. This need to be written as

for i in range(N):
    I[i] <= (main.J1[i] if i < 8 else 0)

@leonardt
Copy link
Collaborator Author

Nasty, I'll open an issue, not sure what the best thing to do about this is besides documentation. Not sure if we can safeguard against it. We could try inspecting the AST and inserting the parenthesis if we can come up with a pattern matching rule that effectively identifies these cases.

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

3 participants