EliminateTargetPaths: don't duplicate modules with only one instance (#1504)

* EliminateTargetPaths: add lone instance test cases

* EliminateTargetPaths: don't rename lone instances

* get rid of trailing comma

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

2 files changed, 104 insertions, 4 deletions

diff --git a/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala b/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala
index e741a32b..00f2ad6a 100644
--- a/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala
+++ b/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala
@@ -155,7 +155,51 @@ class EliminateTargetPaths extends Transform {
       throw NoSuchTargetException(s"""Some targets have illegal paths that cannot be resolved/eliminated: $string""")
     }
 
-    val (newCircuit, renameMap) = run(state.circuit, targets)
+    // get rid of path prefixes of modules with only one instance so we don't rename them
+    val isSingleInstMod: String => Boolean = {
+      val cache = mutable.Map.empty[String, Boolean]
+      mod => cache.getOrElseUpdate(mod, iGraph.findInstancesInHierarchy(mod).size == 1)
+    }
+    val firstRenameMap = RenameMap()
+    val nonSingletonTargets = targets.foldLeft(Seq.empty[IsMember]) {
+      case (acc, t: IsComponent) if t.asPath.nonEmpty =>
+        val origPath = t.asPath
+        val (singletonPrefix, rest) = origPath.partition {
+          case (_, OfModule(mod)) =>
+            isSingleInstMod(mod)
+        }
+
+        if (singletonPrefix.size > 0) {
+          val module = singletonPrefix.last._2.value
+          val circuit = t.circuit
+          val newIsModule =
+            if (rest.isEmpty) {
+              ModuleTarget(circuit, module)
+            } else {
+              val (Instance(inst), OfModule(ofMod)) = rest.last
+              val path = rest.dropRight(1)
+              InstanceTarget(circuit, module, path, inst, ofMod)
+            }
+          val newTarget = t match {
+            case r: ReferenceTarget => r.setPathTarget(newIsModule)
+            case i: InstanceTarget => newIsModule
+          }
+          firstRenameMap.record(t, Seq(newTarget))
+          newTarget +: acc
+        } else {
+          t +: acc
+        }
+      case (acc, t) => t +: acc
+    }
+
+    val (newCircuit, nextRenameMap) = run(state.circuit, nonSingletonTargets)
+
+    val renameMap =
+      if (firstRenameMap.hasChanges) {
+        firstRenameMap andThen nextRenameMap
+      } else {
+        nextRenameMap
+      }
 
     val iGraphx = new InstanceGraph(newCircuit)
     val newlyUnreachableModules = iGraphx.unreachableModules diff iGraph.unreachableModules
diff --git a/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala b/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala
index 5b610890..ba8f1698 100644
--- a/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala
+++ b/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala
@@ -402,20 +402,76 @@ class EliminateTargetPathsSpec extends FirrtlPropSpec with FirrtlMatchers {
     val input =
       """|circuit Foo:
          |  module Baz:
-         |    skip
+         |    node foo = UInt<1>(0)
          |  module Bar:
+         |    node foo = UInt<1>(0)
          |    inst baz of Baz
          |    skip
          |  module Foo:
+         |    node foo = UInt<1>(0)
          |    inst bar of Bar
          |""".stripMargin
     val targets = Seq(
-      CircuitTarget("Foo").module("Foo").instOf("bar", "Bar").instOf("baz", "Baz")
+      CircuitTarget("Foo").module("Foo").instOf("bar", "Bar").instOf("baz", "Baz"),
+      CircuitTarget("Foo").module("Foo").instOf("bar", "Bar"),
+      CircuitTarget("Foo").module("Foo")
     )
-    val output = CircuitState(passes.ToWorkingIR.run(Parser.parse(input)), UnknownForm, Nil)
+    val dontTouches = targets.map(t => DontTouchAnnotation(t.ref("foo")))
+    val inputCircuit = Parser.parse(input)
+    val output = CircuitState(passes.ToWorkingIR.run(inputCircuit), UnknownForm, dontTouches)
       .resolvePaths(targets)
 
     info(output.circuit.serialize)
 
+    output.circuit.serialize should be (inputCircuit.serialize)
+    output.annotations.collect {
+      case a: DontTouchAnnotation => a
+    } should contain allOf (
+      DontTouchAnnotation(ModuleTarget("Foo", "Foo").ref("foo")),
+      DontTouchAnnotation(ModuleTarget("Foo", "Bar").ref("foo")),
+      DontTouchAnnotation(ModuleTarget("Foo", "Baz").ref("foo"))
+    )
+  }
+
+  property("It should not rename parent lone instances but still rename children") {
+    val input =
+      """|circuit FooBar:
+         |  module Bar:
+         |    node baz = UInt<1>(0)
+         |  module FooBar:
+         |    inst foo of Foo
+         |  module Foo:
+         |    inst bar of Bar
+         |    inst barBar of Bar
+         |""".stripMargin
+
+    // modules Foo and FooBar should not be renamed
+    val checks =
+      """circuit FooBar :
+        |  module Foo :
+        |  module FooBar :
+        |    inst foo of Foo""".stripMargin.split("\n")
+
+    val targets = Seq(
+      CircuitTarget("FooBar").module("FooBar").instOf("foo", "Foo").instOf("bar", "Bar"),
+      CircuitTarget("FooBar").module("FooBar").instOf("foo", "Foo").instOf("barBar", "Bar")
+    )
+    val dontTouches = targets.map(t => DontTouchAnnotation(t.ref("baz")))
+    val output = CircuitState(passes.ToWorkingIR.run(Parser.parse(input)), UnknownForm, dontTouches)
+      .resolvePaths(targets)
+
+    info(output.circuit.serialize)
+
+    val outputLines = output.circuit.serialize.split("\n")
+    checks.foreach { line =>
+      outputLines should contain (line)
+    }
+
+    output.annotations.collect {
+      case a: DontTouchAnnotation => a
+    } should contain allOf (
+      DontTouchAnnotation(ModuleTarget("FooBar", "Bar___Foo_bar").ref("baz")),
+      DontTouchAnnotation(ModuleTarget("FooBar", "Bar___Foo_barBar").ref("baz"))
+    )
   }
 }