Initialize ssa decorator #324

leonardt · 2018-12-14T16:28:56Z

This PR is tracking an effort to build up a library of standard decorators that
perform Python -> Python transformations, intended to be used by higher
level DSLs being constructed on top of magma.

This adds a module magma.ssa which contains a decorator @ssa which
is intended to take in a Python function and return a new Python
function in SSA form.

This includes a basic set of test cases in tests/test_ssa.

It also refactors some of the m.circuit.combinational code to be reused
across the two decorators.

Summary: This adds a module `magma.ssa` which contains a decorator `@ssa` which is intended to take in a Python function and return a new Python function in SSA form. This includes a basic set of test cases in `tests/test_ssa` Test Plan: Reviewers: Subscribers: Tasks: Tags: Blame Revision: Differential Revision: https://phabricator.intern.facebook.com/D13458300

phanrahan · 2018-12-14T20:20:28Z

ast example is very cool!

coveralls · 2018-12-14T22:03:05Z

Pull Request Test Coverage Report for Build 1322

100 of 110 (90.91%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 72.472%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
magma/circuit_def.py	8	9	88.89%
magma/ast_utils.py	40	49	81.63%

Totals
Change from base Build 1318:	0.4%
Covered Lines:	3591
Relevant Lines:	4955

💛 - Coveralls

coveralls · 2018-12-14T22:03:06Z

Pull Request Test Coverage Report for Build 1322

100 of 110 (90.91%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 72.472%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
magma/circuit_def.py	8	9	88.89%
magma/ast_utils.py	40	49	81.63%

Totals
Change from base Build 1318:	0.4%
Covered Lines:	3591
Relevant Lines:	4955

💛 - Coveralls

coveralls · 2018-12-14T22:03:06Z

Pull Request Test Coverage Report for Build 1339

112 of 122 (91.8%) changed or added relevant lines in 4 files are covered.
11 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.5%) to 72.561%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
magma/circuit_def.py	8	9	88.89%
magma/ast_utils.py	40	49	81.63%

Files with Coverage Reduction	New Missed Lines	%
magma/fromverilog.py	11	86.67%

Totals
Change from base Build 1318:	0.5%
Covered Lines:	3607
Relevant Lines:	4971

💛 - Coveralls

coveralls · 2018-12-14T22:03:06Z

Pull Request Test Coverage Report for Build 1322

100 of 110 (90.91%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 72.472%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
magma/circuit_def.py	8	9	88.89%
magma/ast_utils.py	40	49	81.63%

Totals
Change from base Build 1318:	0.4%
Covered Lines:	3591
Relevant Lines:	4955

💛 - Coveralls

rsetaluri · 2018-12-17T21:58:21Z

Cool! Is this meant to be used in conjunction with the combinational decorator? If so can you include an example of how they would both be used and what a useful use case/target output is?

hofstee · 2018-12-20T07:08:13Z

magma/ssa/ssa.py

+    def visit_Name(self, node):
+        if isinstance(node.ctx, ast.Store):
+            self.var_counter[node.id] += 1
+        if isinstance(node.ctx, ast.Store) or node.id in self.var_counter:


if isinstance(node.ctx, ast.Store) or node.id in self.var_counter:

Redundant check on if it's a store, no?

hofstee · 2018-12-20T07:13:10Z

magma/ssa/ssa.py

+        node.body = flatten([self.visit(s) for s in node.body])
+        return node
+
+    def visit_Name(self, node):


You might want to rename inputs.

For example, this is handled incorrectly:

@ssa def default(I: m.Bits(2), x_0: m.Bit) -> m.Bit: x = I[1] if x_0: x = I[0] return x

If you want to preserve interface names, I would suggest inserting input renamings as the first thing in the SSA-ified code, so the output might reasonably look like this:

def default(I: m.Bits(2), x_0: m.Bit) -> m.Bit: I_0, x_0_0 = I, x_0 # this is done in a single tuple assign to avoid things like def default(x_0: m.Bits(2), x_0_0: m.Bit), which would make x_0_0 = x_0, x_0_0_0 = x_0_0, which is also wrong # rest of ssa translated code here using new input names...

The first example I posted generates this right now:

def default(I: m.Bits(2), x_0: m.Bit) ->m.Bit: x_0 = I[1] x_1 = I[0] x_2 = phi([x_0, x_1], x_0) return x_2

Here's a test for the edge case I mentioned in the second comment:

@ssa def default(x_0: m.Bits(2), x_0_0: m.Bit) -> m.Bit: x = x_0[1] if x_0_0: x = x_0[0] return x

Just going to piggyback on this and say it might be a bigger issue than you think, if this is intended to be used in conjunction with other passes, I wouldn't be surprised if there would be renaming of variables happening in other passes.

For example, creating a wrapper module - I might rename the wires on the internal one by appending _0 to the inputs if I need to modify the wires before feeding them into the internal module. Rigel did something like this. Combined with the ssa pass things would break in ways that would be pretty odd.

hofstee · 2018-12-20T07:35:25Z

magma/ssa/ssa.py

+            node.id += f"_{self.var_counter[node.id]}"
+        return node
+
+    def visit_If(self, node):


I think this (the visit_If function) might be made simpler if you kept track of two things, a dict with the counters for stores, and a dict mapping the variable to its last name for loads.

This way you would preserve the store dict across both branches in the if, and reset the load mapping dict for each branch. This would also eliminate the need to go through everything in the false_var_counter and remap variable store names.

The phi logic would still need to be roughly the same though, but you would have the two load mappings and then you would insert a phi for every entry where the last load name is different between the two dicts for a variable.

Here's another bug case I think:

def test_wat(): @ssa def basic_if(I: m.Bit, S: m.Bit) -> m.Bit: x = I if S: x = x else: x = x return x print(inspect.getsource(basic_if)) assert inspect.getsource(basic_if) == """\ def basic_if(I: m.Bits(2), S: m.Bit) ->m.Bit: x_0 = I x_1 = x_0 x_2 = x_0 x_3 = phi([x_2, x_1], S) return x_3 """

Should this phi be the other way around?

def test_default(): @ssa def default(I: m.Bits(2), S: m.Bit) -> m.Bit: x = I[1] if S: x = I[0] return x assert inspect.getsource(default) == """\ def default(I: m.Bits(2), S: m.Bit) ->m.Bit: x_0 = I[1] x_1 = I[0] x_2 = phi([x_0, x_1], S) return x_2 """

Nevermind on that last point.

hofstee · 2019-01-09T00:55:12Z

magma/ssa/ssa.py

+            node.id += f"_{self.var_counter[node.id]}"
+        return node
+
+    def visit_If(self, node):


Should this phi be the other way around?

def test_default(): @ssa def default(I: m.Bits(2), S: m.Bit) -> m.Bit: x = I[1] if S: x = I[0] return x assert inspect.getsource(default) == """\ def default(I: m.Bits(2), S: m.Bit) ->m.Bit: x_0 = I[1] x_1 = I[0] x_2 = phi([x_0, x_1], S) return x_2 """

leonardt · 2019-01-11T22:05:57Z

@THofstee looks like the tests pass when you renamed the inputs, but that's because it only checks that the generated code makes sense. The changes you pushed look good.

I think this is okay, since this logic/tests are self contained in the sense that they don't test how this affects use with magma, but that's by design (in case someone wants to use this for some other DSL). Basically, I think any magma logic (say m.circuit.combinational) should be aware that the ssa transform will change the input names. It's the responsibility of the user to handle this issue (so as a first pass m.circuit.combinational could generate wrappers to handle the name change, and then we can add better logic to remove the need for wrappers in the generated code so it is less verbose).

Can you confirm if you think the input name issue has been resolved? Are there are any other issues we should be looking in to? Otherwise, I vote we merge and start using it and open up specific issues as they arise.

hofstee · 2019-01-11T23:23:04Z

I addressed all the comments I made in my review so I don't think there's anything else for the time being.

leonardt requested review from phanrahan, hofstee and rsetaluri December 14, 2018 16:28

Fix issue with combinational

426179d

hofstee requested changes Dec 21, 2018

View reviewed changes

hofstee requested changes Jan 9, 2019

View reviewed changes

hofstee added 3 commits January 9, 2019 01:01

rewrite ssa to use name mapping

6199f5a

remove true_name initialization

75cacc2

rename inputs and see what breaks

a93d5d0

hofstee merged commit 2cae041 into master Jan 12, 2019

leonardt deleted the ssa branch February 5, 2019 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize ssa decorator #324

Initialize ssa decorator #324

leonardt commented Dec 14, 2018

phanrahan commented Dec 14, 2018

coveralls commented Dec 14, 2018

coveralls commented Dec 14, 2018

coveralls commented Dec 14, 2018 •

edited

coveralls commented Dec 14, 2018

rsetaluri commented Dec 17, 2018

hofstee Dec 20, 2018

hofstee Dec 20, 2018

hofstee Dec 20, 2018

hofstee Dec 20, 2018

hofstee Dec 20, 2018

hofstee Dec 20, 2018

hofstee Dec 20, 2018 •

edited

hofstee Jan 8, 2019

hofstee Jan 9, 2019 •

edited

hofstee Jan 9, 2019

hofstee Jan 9, 2019 •

edited

leonardt commented Jan 11, 2019

hofstee commented Jan 11, 2019

Initialize ssa decorator #324

Initialize ssa decorator #324

Conversation

leonardt commented Dec 14, 2018

phanrahan commented Dec 14, 2018

coveralls commented Dec 14, 2018

Pull Request Test Coverage Report for Build 1322

💛 - Coveralls

coveralls commented Dec 14, 2018

Pull Request Test Coverage Report for Build 1322

💛 - Coveralls

coveralls commented Dec 14, 2018 • edited

Pull Request Test Coverage Report for Build 1339

💛 - Coveralls

coveralls commented Dec 14, 2018

Pull Request Test Coverage Report for Build 1322

💛 - Coveralls

rsetaluri commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hofstee Dec 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hofstee Jan 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hofstee Jan 9, 2019 • edited

Choose a reason for hiding this comment

leonardt commented Jan 11, 2019

hofstee commented Jan 11, 2019

coveralls commented Dec 14, 2018 •

edited

hofstee Dec 20, 2018 •

edited

hofstee Jan 9, 2019 •

edited

hofstee Jan 9, 2019 •

edited