Remove new unreachables in EliminateTargetPaths

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>

2 files changed, 51 insertions, 31 deletions

diff --git a/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala b/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala
index e40b80f8..91f0f3dc 100644
--- a/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala
+++ b/src/main/scala/firrtl/annotations/transforms/EliminateTargetPaths.scala
@@ -5,7 +5,7 @@ package firrtl.annotations.transforms
 import firrtl.Mappers._
 import firrtl.analyses.InstanceGraph
 import firrtl.annotations.ModuleTarget
-import firrtl.annotations.TargetToken.{Instance, OfModule}
+import firrtl.annotations.TargetToken.{Instance, OfModule, fromDefModuleToTargetToken}
 import firrtl.annotations.analysis.DuplicationHelper
 import firrtl.annotations._
 import firrtl.ir._
@@ -54,22 +54,16 @@ class EliminateTargetPaths extends Transform {
     * @param s
     * @return
     */
-  private def onStmt(dupMap: DuplicationHelper,
-                     oldUsedOfModules: mutable.HashSet[String],
-                     newUsedOfModules: mutable.HashSet[String])
+  private def onStmt(dupMap: DuplicationHelper)
                     (originalModule: String, newModule: String)
                     (s: Statement): Statement = s match {
     case d@DefInstance(_, name, module) =>
       val ofModule = dupMap.getNewOfModule(originalModule, newModule, Instance(name), OfModule(module)).value
-      newUsedOfModules += ofModule
-      oldUsedOfModules += module
       d.copy(module = ofModule)
     case d@WDefInstance(_, name, module, _) =>
       val ofModule = dupMap.getNewOfModule(originalModule, newModule, Instance(name), OfModule(module)).value
-      newUsedOfModules += ofModule
-      oldUsedOfModules += module
       d.copy(module = ofModule)
-    case other => other map onStmt(dupMap, oldUsedOfModules, newUsedOfModules)(originalModule, newModule)
+    case other => other map onStmt(dupMap)(originalModule, newModule)
   }
 
   /** Returns a modified circuit and [[RenameMap]] containing the associated target remapping
@@ -84,14 +78,6 @@ class EliminateTargetPaths extends Transform {
     // For each target, record its path and calculate the necessary modifications to circuit
     targets.foreach { t => dupMap.expandHierarchy(t) }
 
-    // Records original list of used ofModules
-    val oldUsedOfModules = mutable.HashSet[String]()
-    oldUsedOfModules += cir.main
-
-    // Records new list of used ofModules
-    val newUsedOfModules = mutable.HashSet[String]()
-    newUsedOfModules += cir.main
-
     // Contains new list of module declarations
     val duplicatedModuleList = mutable.ArrayBuffer[DefModule]()
 
@@ -102,19 +88,13 @@ class EliminateTargetPaths extends Transform {
         val newM = m match {
           case e: ExtModule => e.copy(name = newName)
           case o: Module =>
-            o.copy(name = newName, body = onStmt(dupMap, oldUsedOfModules, newUsedOfModules)(m.name, newName)(o.body))
+            o.copy(name = newName, body = onStmt(dupMap)(m.name, newName)(o.body))
         }
         duplicatedModuleList += newM
       }
     }
 
-    // Calculate the final module list
-    // A module is in the final list if:
-    // 1) it is a module that is instantiated (new or old)
-    // 2) it is an old module that was not instantiated and is still not instantiated
-    val finalModuleList = duplicatedModuleList.filter(m =>
-      newUsedOfModules.contains(m.name) || (!newUsedOfModules.contains(m.name) && !oldUsedOfModules.contains(m.name))
-    )
+    val finalModuleList = duplicatedModuleList
     lazy val finalModuleSet = finalModuleList.map{ case a: DefModule => a.name }.toSet
 
     // Records how targets have been renamed
@@ -125,7 +105,7 @@ class EliminateTargetPaths extends Transform {
      */
     targets.foreach { t =>
       val newTsx = dupMap.makePathless(t)
-      val newTs = newTsx.filter(c => newUsedOfModules.contains(c.moduleOpt.get))
+      val newTs = newTsx
       if(newTs.nonEmpty) {
         renameMap.record(t, newTs)
         val m = Target.referringModule(t)
@@ -134,7 +114,7 @@ class EliminateTargetPaths extends Transform {
           case a: ModuleTarget if finalModuleSet(a.module) => Some(a)
           case _                                           => None
         }
-        renameMap.record(m, (duplicatedModules ++ oldModule).distinct)
+        renameMap.record(m, (duplicatedModules).distinct)
       }
     }
 
@@ -153,7 +133,8 @@ class EliminateTargetPaths extends Transform {
     val targets = annotations.map(_.asInstanceOf[ResolvePaths]).flatMap(_.targets.collect { case x: IsMember => x })
 
     // Check validity of paths in targets
-    val instanceOfModules = new InstanceGraph(state.circuit).getChildrenInstanceOfModule
+    val iGraph = new InstanceGraph(state.circuit)
+    val instanceOfModules = iGraph.getChildrenInstanceOfModule
     val targetsWithInvalidPaths = mutable.ArrayBuffer[IsMember]()
     targets.foreach { t =>
       val path = t match {
@@ -176,6 +157,23 @@ class EliminateTargetPaths extends Transform {
 
     val (newCircuit, renameMap) = run(state.circuit, targets)
 
-    state.copy(circuit = newCircuit, renames = Some(renameMap), annotations = annotationsx)
+    val iGraphx = new InstanceGraph(newCircuit)
+    val newlyUnreachableModules = iGraphx.unreachableModules diff iGraph.unreachableModules
+
+    val newCircuitGC = {
+      val modulesx = newCircuit.modules.flatMap{
+        case dead if newlyUnreachableModules(dead.OfModule) => None
+        case live =>
+          val m = CircuitTarget(newCircuit.main).module(live.name)
+          renameMap.get(m).foreach(_ => renameMap.record(m, m))
+          Some(live)
+      }
+      newCircuit.copy(modules = modulesx)
+    }
+
+    logger.info("Renames:")
+    logger.info(renameMap.serialize)
+
+    state.copy(circuit = newCircuitGC, renames = Some(renameMap), annotations = annotationsx)
   }
 }
diff --git a/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala b/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala
index 9d7df718..11c40d5f 100644
--- a/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala
+++ b/src/test/scala/firrtlTests/annotationTests/EliminateTargetPathsSpec.scala
@@ -380,7 +380,8 @@ class EliminateTargetPathsSpec extends FirrtlPropSpec with FirrtlMatchers {
          |    node x = UInt<1>(0)
          |    skip
          |  module Foo:
-         |    inst bar of Bar""".stripMargin
+         |    inst bar of Bar
+         |    inst baz of Bar""".stripMargin
     val Bar_x = CircuitTarget("Foo").module("Bar").ref("x")
     val output = CircuitState(passes.ToWorkingIR.run(Parser.parse(input)), UnknownForm, Seq(DontTouchAnnotation(Bar_x)))
       .resolvePaths(Seq(CircuitTarget("Foo").module("Foo").instOf("bar", "Bar")))
@@ -394,6 +395,27 @@ class EliminateTargetPathsSpec extends FirrtlPropSpec with FirrtlMatchers {
       .filter{
         case _: DeletedAnnotation => false
         case _ => true
-      } should contain (DontTouchAnnotation(newBar_x))
+      } should contain allOf (DontTouchAnnotation(newBar_x), DontTouchAnnotation(Bar_x))
+  }
+
+  property("It should not rename lone instances") {
+    val input =
+      """|circuit Foo:
+         |  module Baz:
+         |    skip
+         |  module Bar:
+         |    inst baz of Baz
+         |    skip
+         |  module Foo:
+         |    inst bar of Bar
+         |""".stripMargin
+    val targets = Seq(
+      CircuitTarget("Foo").module("Foo").instOf("bar", "Bar").instOf("baz", "Baz")
+    )
+    val output = CircuitState(passes.ToWorkingIR.run(Parser.parse(input)), UnknownForm, Nil)
+      .resolvePaths(targets)
+
+    info(output.circuit.serialize)
+
   }
 }