Merge pull request #516 from phanrahan/fix-uniq

Remove unnec. rename in uniq. pass

magma/uniquification.py

@@ -36,8 +36,7 @@ def _hash(definition):
 
 class UniquificationPass(DefinitionPass):
     def __init__(self, main, mode):
-        super(UniquificationPass, self).__init__(main)
-        self.definitions = {}
+        super().__init__(main)
         self.mode = mode
         self.seen = {}
         self.original_names = {}
@@ -46,37 +45,22 @@ def __call__(self, definition):
         name = definition.name
         key = _hash(definition)
 
-        if name not in self.seen:
-            self.seen[name] = {}
-        if key not in self.seen[name]:
-            if self.mode is UniquificationMode.UNIQUIFY and len(self.seen[name]) > 0:
-                new_name = name + "_unq" + str(len(self.seen[name]))
+        seen = self.seen.setdefault(name, {})
+        if key not in seen:
+            if self.mode is UniquificationMode.UNIQUIFY and len(seen) > 0:
+                new_name = name + "_unq" + str(len(seen))
                 type(definition).rename(definition, new_name)
-            self.seen[name][key] = [definition]
+            seen[key] = [definition]
         else:
             if self.mode is not UniquificationMode.UNIQUIFY:
-                assert self.seen[name][key][0].name == definition.name
-            else:
-                type(definition).rename(definition, self.seen[name][key][0].name)
-            self.seen[name][key].append(definition)
-
-    def _run(self, definition):
-        if not isdefinition(definition):
-            return
-
-        for instance in definition.instances:
-            instancedefinition = type(instance)
-            if isdefinition(instancedefinition):
-                self._run( instancedefinition )
-
-        id_ = id(definition)
-        if id_ in self.definitions:
-            return
-        self.definitions[id_] = definition
-        self(definition)
+                assert seen[key][0].name == name
+            elif name != seen[key][0].name:
+                new_name = seen[key][0].name
+                type(definition).rename(definition, new_name)
+            seen[key].append(definition)
 
     def run(self):
-        super(UniquificationPass, self).run()
+        super().run()
         duplicated = []
         for name, definitions in self.seen.items():
             if len(definitions) > 1:

tests/gold/uniquify_multiple_rename.json

@@ -0,0 +1,55 @@
+{"top":"global._Top",
+"namespaces":{
+  "global":{
+    "modules":{
+      "Foo":{
+        "type":["Record",[
+          ["I",["Array",2,"BitIn"]],
+          ["O",["Array",2,"Bit"]]
+        ]],
+        "connections":[
+          ["self.O","self.I"]
+        ]
+      },
+      "Foo_unq1":{
+        "type":["Record",[
+          ["I",["Array",3,"BitIn"]],
+          ["O",["Array",3,"Bit"]]
+        ]],
+        "connections":[
+          ["self.O","self.I"]
+        ]
+      },
+      "_Top":{
+        "type":["Record",[
+          ["I0",["Array",2,"BitIn"]],
+          ["I1",["Array",3,"BitIn"]],
+          ["I2",["Array",3,"BitIn"]],
+          ["O0",["Array",2,"Bit"]],
+          ["O1",["Array",3,"Bit"]],
+          ["O2",["Array",3,"Bit"]]
+        ]],
+        "instances":{
+          "Foo_inst0":{
+            "modref":"global.Foo"
+          },
+          "Foo_inst1":{
+            "modref":"global.Foo_unq1"
+          },
+          "Foo_inst2":{
+            "modref":"global.Foo_unq1"
+          }
+        },
+        "connections":[
+          ["self.I0","Foo_inst0.I"],
+          ["self.O0","Foo_inst0.O"],
+          ["self.I1","Foo_inst1.I"],
+          ["self.O1","Foo_inst1.O"],
+          ["self.I2","Foo_inst2.I"],
+          ["self.O2","Foo_inst2.O"]
+        ]
+      }
+    }
+  }
+}
+}

tests/test_uniquify.py

@@ -4,7 +4,7 @@
 
 
 def test_verilog_field_uniquify():
-    # https://github.com/phanrahan/magma/issues/330
+    # https://github.com/phanrahan/magma/issues/330.
     HalfAdder = m.DefineCircuit('HalfAdder', 'A', m.In(m.Bit), 'B',
                                 m.In(m.Bit), 'S', m.Out(m.Bit), 'C',
                                 m.Out(m.Bit))
@@ -14,6 +14,7 @@ def test_verilog_field_uniquify():
     '''
     m.EndCircuit()
 
+
 def test_uniquify_equal():
     foo = m.DefineCircuit("foo", "I", m.In(m.Bit), "O", m.Out(m.Bit))
     m.wire(foo.I, foo.O)
@@ -173,11 +174,108 @@ def test_hash_verilog():
     # Run the uniquification pass as a mechanism to check that foo0 and foo1
     # hash to two different things even though they have the same repr.
     pass_ = m.UniquificationPass(top, None)
-    pass_._run(top)
+    pass_.run()
     foo_seen = pass_.seen["foo"]
     assert len(foo_seen) == 2
     for v in foo_seen.values():
         assert len(v) == 1
     expected_ids_ = {id(v[0]) for v in foo_seen.values()}
     ids_ = {id(foo0), id(foo1)}
     assert expected_ids_ == ids_
+
+
+def test_same_verilog():
+    """
+    This test checks that if we have multiple verilog-wrapped circuits with the
+    same name and same source, the uniquification pass does *not* try to perform
+    a rename. As it does not need to.
+    """
+    def _generate_foo():
+        return m.DefineFromVerilog("""
+module foo(input i, output o);
+    assign o = i;
+endmodule""")[0]
+
+
+    class _Cell0(m.Circuit):
+        IO = ["I", m.In(m.Bit), "O", m.Out(m.Bit)]
+
+        @classmethod
+        def definition(io):
+            foo = _generate_foo()()
+            foo.i <= io.I
+            io.O <= foo.o
+
+
+    class _Cell1(m.Circuit):
+        IO = ["I", m.In(m.Bit), "O", m.Out(m.Bit)]
+
+        @classmethod
+        def definition(io):
+            foo = _generate_foo()()
+            foo.i <= io.I
+            io.O <= foo.o
+
+
+    class _Top(m.Circuit):
+        IO = ["I", m.In(m.Bits[2]), "O", m.Out(m.Bits[2])]
+
+        @classmethod
+        def definition(io):
+            cell0 = _Cell0()
+            cell1 = _Cell1()
+            cell0.I <= io.I[0]
+            cell1.I <= io.I[1]
+            io.O[0] <= cell0.O
+            io.O[1] <= cell1.O
+
+    # Check that uniq. pass runs successfully.
+    pass_ = m.UniquificationPass(_Top, None)
+    pass_.run()
+
+
+def test_multiple_renamed():
+    def _gen_foo(width):
+        class Foo(m.Circuit):
+            IO = ["I", m.In(m.Bits[width]), "O", m.Out(m.Bits[width])]
+
+            @classmethod
+            def definition(io):
+                io. O <= io.I
+
+        return Foo
+
+    Foo0 = _gen_foo(2)
+    Foo1 = _gen_foo(3)
+    Foo2 = _gen_foo(3)
+
+    class _Top(m.Circuit):
+        IO = [
+            "I0", m.In(m.Bits[2]),
+            "I1", m.In(m.Bits[3]),
+            "I2", m.In(m.Bits[3]),
+            "O0", m.Out(m.Bits[2]),
+            "O1", m.Out(m.Bits[3]),
+            "O2", m.Out(m.Bits[3]),
+        ]
+
+        @classmethod
+        def definition(io):
+            foo0 = Foo0()
+            foo1 = Foo1()
+            foo2 = Foo2()
+
+            foo0.I <= io.I0
+            io.O0 <= foo0.O
+
+            foo1.I <= io.I1
+            io.O1 <= foo1.O
+
+            foo2.I <= io.I2
+            io.O2 <= foo2.O
+
+    BASENAME = "uniquify_multiple_rename"
+    m.compile(f"build/{BASENAME}", _Top, output="coreir")
+    assert check_files_equal(__file__,
+                             f"build/{BASENAME}.json",
+                             f"gold/{BASENAME}.json")