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

Preliminary support for if statements and ternary expressions #224

Merged
merged 33 commits into from Aug 2, 2018

Conversation

leonardt
Copy link
Collaborator

Supports basic nesting. Includes tests that compile some Mux declarations to verilog.

Things that aren't supported:

  • Using anything other than an assignment statement in the if/else body
  • Assigning to a variable only once in the if or else body (not both). We could support this if the variable is already defined in the enclosing scope, for example using a default value
    x = 3
    if S:
       x = 4
    
    This brings up another issue, which is that it doesn't support a default value. (So the above code would break even if x was assigned in the else block.
  • If without an else (for the same reason as the above)

These changes introduce a decorator @m.circuit_def, which is replaces @classmethod by first doing a rewrite pass. This provides hooks for doing other syntactic rewrites.

Right now it uses = syntax to bind the result of the Mux that is generated for the statements. We could explore binding directly as an alternative to wiring (or by using <= syntax to bind to a port), but the existing mechanism required the least amount of changes (we bind a variable to a magma value, just like any other magma code, which is then wired up to a port).

Thoughts/feedback? Should I continue to widen support for the varying cases mentioned above before we merge? Or should we get this in so people can start playing around with it.

@coveralls
Copy link

coveralls commented Jul 28, 2018

Pull Request Test Coverage Report for Build 816

  • 63 of 70 (90.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 69.357%

Changes Missing Coverage Covered Lines Changed/Added Lines %
magma/circuit_def.py 60 67 89.55%
Totals Coverage Status
Change from base Build 802: 0.4%
Covered Lines: 3076
Relevant Lines: 4435

💛 - Coveralls

Copy link
Collaborator

@rsetaluri rsetaluri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I actually think the syntax and covered cases are good as is. If its marked very clearly as an experimental feature then I think it's okay to merge as is. Also the what works vs what doesn't work you note in your comment should be provided somewhere in magma docs so users know what to expect.

A question: what is allowed in the test part of the if? From the code it didn't seem like you do any checks there.

@leonardt
Copy link
Collaborator Author

Added some documentation in doc/circuit_definitions.md. It also includes a note that the test can be any expression that evaluates to a magma Bit value, so something like io.S or io.S[0] or eq(io.I0, io.I1). Basically, the rewrite rules change the expression into an input to the Mux, at which point the standard magma typechecker will kick in (enforcing this constraint). We could do a syntax level analysis to do a check, but this would require type inferencing rules. Another approach might be to catch an error in running the magma code, and a trace it back to the original if statement.

@phanrahan
Copy link
Owner

Cool!

A couple of comments

  1. I suggest we name Mux2xn to just Mux2 if width is None.

  2. I believe you suggested m.circuit.combinational for the decorator when
    we met. I can imagine we will have more than one decorator, so it might be
    worth doing this now.

@leonardt
Copy link
Collaborator Author

Done

@phanrahan
Copy link
Owner

Can you handle multiple assignments inside a block?

@leonardt
Copy link
Collaborator Author

leonardt commented Jul 29, 2018

It takes the last assignment, and prints out a warning saying that you've assigned multiple times within the same block (and that it's taking the last assignment). It's a simple change to make it an error if we want.

@leonardt leonardt merged commit 24083ef into master Aug 2, 2018
@leonardt leonardt deleted the if-statements 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