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 wire in sequential test #958

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add wire in sequential test #958

wants to merge 1 commit into from

Conversation

leonardt
Copy link
Collaborator

So this works given a simple patch to ast_tools:

diff --git a/ast_tools/passes/ssa.py b/ast_tools/passes/ssa.py
index 01d90fa..c8dc92e 100644
--- a/ast_tools/passes/ssa.py
+++ b/ast_tools/passes/ssa.py
@@ -653,7 +653,7 @@ class ssa(Pass):
         names_to_attr = {}
         seen = set()
 
-        for written_attr in writter_attr_visitor.written_attrs:
+        for written_attr in {}:
             d_attr = DeepNode(written_attr)
             if d_attr in seen:
                 continue

Basically, the expression context provider used in the written attrs visitor treats augassign as a write, so it tries to rewrite these wiring statements. There's a few options here:
(1) patch the code to ignore wiring (don't treat it like asssign), this probably means either extending the expressioncontextprovider to only treat assign as store, or writing our own provider that does the same.
(2) patch the code to support augassign. This requires as few changes, basically there's places where the attr logic assumes names, but here we have nested attributes (e.g. self.x.I), so we need to support nesting for port references. We also need to adapt the logic to handle the lack of default values (i.e. there's not necessarily a self.x.O that corresponds to the default value of self.x.I, like we assume now for attrs (since they're implicit registers).
(3) patch the code to only support wiring with augassign (i.e. drop support for current attrs), the code should look similar with just the default value logic removed. The idea here we had is to have the implicit register syntax handled as a pre-pass that rewrites it into this wiring style with the default value.

Option (1) maintains backwards compatibility, but doesn't allow support for wiring inside if statements (since they are ignored in the ssa logic).
Option (2) maintains backwards compatibility and adds support for wiring inside if statements
Option (3) breaks backwards compatibility (we need to add the register pre-pass), but adds support for wiring inside if statements and avoids having to handle/maintain two different cases in the code.

I think based on discussion on Slack, we want to go with Option (3), while Option (1) would present a simple way forward if we don't need to support wiring inside if statements yet

@leonardt leonardt requested a review from cdonovick May 25, 2021 18:05
@cdonovick
Copy link
Collaborator

I'll look at option 1 see how hard it is. Otherwise I will probably go with option 3.

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.

2 participants