Don't Dedup modules if it would change semantics (#1713)

If a module has ports of type Bundle that are used in aggregate
connections in parent modules, Dedup cannot change the names of the
fields of the Bundle or it would change the semantics of the connection.
Dedup now detects this case and refrains from agnostifying the ports of
such modules to prevent this issue.

2 files changed, 118 insertions, 6 deletions

diff --git a/src/main/scala/firrtl/transforms/Dedup.scala b/src/main/scala/firrtl/transforms/Dedup.scala
index 04ac968d..dc182858 100644
--- a/src/main/scala/firrtl/transforms/Dedup.scala
+++ b/src/main/scala/firrtl/transforms/Dedup.scala
@@ -5,15 +5,18 @@ package transforms
 
 import firrtl.ir._
 import firrtl.Mappers._
+import firrtl.traversals.Foreachers._
 import firrtl.analyses.InstanceGraph
 import firrtl.annotations._
 import firrtl.passes.{InferTypes, MemPortUtils}
-import firrtl.Utils.throwInternalError
+import firrtl.Utils.{kind, splitRef, throwInternalError}
 import firrtl.annotations.transforms.DupedResult
 import firrtl.annotations.TargetToken.{OfModule, Instance}
 import firrtl.options.{HasShellOptions, ShellOption}
 import logger.LazyLogging
 
+import scala.annotation.tailrec
+
 // Datastructures
 import scala.collection.mutable
 
@@ -446,6 +449,48 @@ object DedupModules extends LazyLogging {
     changeInternals({n => n}, retype, {i => i}, renameOfModule)(module)
   }
 
+  @tailrec
+  private def hasBundleType(tpe: Type): Boolean = tpe match {
+    case _: BundleType => true
+    case _: GroundType => false
+    case VectorType(t, _) => hasBundleType(t)
+  }
+
+  // Find modules that should not have their ports agnostified to avoid bug in
+  // https://github.com/freechipsproject/firrtl/issues/1703
+  // Marks modules that have a port of BundleType that are connected via an aggregate connect or
+  // partial connect in an instantiating parent
+  // Order of modules does not matter
+  private def modsToNotAgnostifyPorts(modules: Seq[DefModule]): Set[String] = {
+    val dontDedup = mutable.HashSet.empty[String]
+    def onModule(mod: DefModule): Unit = {
+      val instToModule = mutable.HashMap.empty[String, String]
+      def markAggregatePorts(expr: Expression): Unit = {
+        if (kind(expr) == InstanceKind && hasBundleType(expr.tpe)) {
+          val (WRef(inst, _, _, _), _) = splitRef(expr)
+          dontDedup += instToModule(inst)
+        }
+      }
+      def onStmt(stmt: Statement): Unit = {
+        stmt.foreach(onStmt)
+        stmt match {
+          case inst: DefInstance =>
+            instToModule(inst.name) = inst.module
+          case Connect(_, lhs, rhs) =>
+            markAggregatePorts(lhs)
+            markAggregatePorts(rhs)
+          case PartialConnect(_, lhs, rhs) =>
+            markAggregatePorts(lhs)
+            markAggregatePorts(rhs)
+          case _ =>
+        }
+      }
+      mod.foreach(onStmt)
+    }
+    modules.foreach(onModule)
+    dontDedup.toSet
+  }
+
   //scalastyle:off
   /** Returns
     *  1) map of tag to all matching module names,
@@ -470,6 +515,8 @@ object DedupModules extends LazyLogging {
 
     val agnosticRename = RenameMap()
 
+    val dontAgnostifyPorts = modsToNotAgnostifyPorts(moduleLinearization)
+
     moduleLinearization.foreach { originalModule =>
       // Replace instance references to new deduped modules
       val dontcare = RenameMap()
@@ -487,7 +534,13 @@ object DedupModules extends LazyLogging {
 
         // Build tag
         val builder = new mutable.ArrayBuffer[Any]()
-        agnosticModule.ports.foreach { builder ++= _.serialize }
+
+        // It may seem weird to use non-agnostified ports with an agnostified body because
+        // technically it would be invalid FIRRTL, but it is logically sound for the purpose of
+        // calculating deduplication tags
+        val ports =
+          if (dontAgnostifyPorts(originalModule.name)) originalModule.ports else agnosticModule.ports
+        ports.foreach { builder ++= _.serialize }
 
         agnosticModule match {
           case Module(i, n, ps, b) => builder ++= fastSerializedHash(b).toString()//.serialize
diff --git a/src/test/scala/firrtlTests/transforms/DedupTests.scala b/src/test/scala/firrtlTests/transforms/DedupTests.scala
index bb12c759..5776db31 100644
--- a/src/test/scala/firrtlTests/transforms/DedupTests.scala
+++ b/src/test/scala/firrtlTests/transforms/DedupTests.scala
@@ -253,6 +253,61 @@ class DedupModuleTests extends HighTransformSpec {
      val diff_params = mkfir(("BB", "BB"), ("0", "1"))
      execute(diff_params, diff_params, Seq.empty)
   }
+
+  "Modules with aggregate ports that are bulk connected" should "NOT dedup if their port names differ" in {
+    val input =
+      """
+        |circuit FooAndBarModule :
+        |  module FooModule :
+        |    output io : {flip foo : UInt<1>, fuzz : UInt<1>}
+        |    io.fuzz <= io.foo
+        |  module BarModule :
+        |    output io : {flip bar : UInt<1>, buzz : UInt<1>}
+        |    io.buzz <= io.bar
+        |  module FooAndBarModule :
+        |    output io : {foo : {flip foo : UInt<1>, fuzz : UInt<1>}, bar : {flip bar : UInt<1>, buzz : UInt<1>}}
+        |    inst foo of FooModule
+        |    inst bar of BarModule
+        |    io.foo <- foo.io
+        |    io.bar <- bar.io
+        |""".stripMargin
+    val check = input
+    execute(input, check, Seq.empty)
+  }
+
+  "Modules with aggregate ports that are bulk connected" should "dedup if their port names are the same" in {
+    val input =
+      """
+        |circuit FooAndBarModule :
+        |  module FooModule :
+        |    output io : {flip foo : UInt<1>, fuzz : UInt<1>}
+        |    io.fuzz <= io.foo
+        |  module BarModule :
+        |    output io : {flip foo : UInt<1>, fuzz : UInt<1>}
+        |    io.fuzz <= io.foo
+        |  module FooAndBarModule :
+        |    output io : {foo : {flip foo : UInt<1>, fuzz : UInt<1>}, bar : {flip bar : UInt<1>, buzz : UInt<1>}}
+        |    inst foo of FooModule
+        |    inst bar of BarModule
+        |    io.foo <- foo.io
+        |    io.bar <- bar.io
+        |""".stripMargin
+    val check =
+      """
+        |circuit FooAndBarModule :
+        |  module FooModule :
+        |    output io : {flip foo : UInt<1>, fuzz : UInt<1>}
+        |    io.fuzz <= io.foo
+        |  module FooAndBarModule :
+        |    output io : {foo : {flip foo : UInt<1>, fuzz : UInt<1>}, bar : {flip bar : UInt<1>, buzz : UInt<1>}}
+        |    inst foo of FooModule
+        |    inst bar of FooModule
+        |    io.foo <- foo.io
+        |    io.bar <- bar.io
+        |""".stripMargin
+    execute(input, check, Seq.empty)
+  }
+
   "The module A and B" should "be deduped with the first module in order" in {
     val input =
       """circuit Top :
@@ -772,11 +827,15 @@ class DedupModuleTests extends HighTransformSpec {
       |    output oa: {z: {y: {x: UInt<1>}}, a: UInt<1>}
       |    output ob: {a: {b: {c: UInt<1>}}, z: UInt<1>}
       |    inst a of a
-      |    a.i <= ia
-      |    oa <= a.o
+      |    a.i.z.y.x <= ia.z.y.x
+      |    a.i.a <= ia.a
+      |    oa.z.y.x <= a.o.z.y.x
+      |    oa.a <= a.o.a
       |    inst b of b
-      |    b.q <= ib
-      |    ob <= b.r
+      |    b.q.a.b.c <= ib.a.b.c
+      |    b.q.z <= ib.z
+      |    ob.a.b.c <= b.r.a.b.c
+      |    ob.z <= b.r.z
       |  module a:
       |    input i: {z: {y: {x: UInt<1>}}, a: UInt<1>}
       |    output o: {z: {y: {x: UInt<1>}}, a: UInt<1>}