Merge pull request #1638 from freechipsproject/manipulate-names-issues-refactor

Refactor ManipulateNames to use Target

3 files changed, 415 insertions, 6 deletions

diff --git a/src/test/scala/firrtlTests/VerilogEmitterTests.scala b/src/test/scala/firrtlTests/VerilogEmitterTests.scala
index ae06c331..171bce78 100644
--- a/src/test/scala/firrtlTests/VerilogEmitterTests.scala
+++ b/src/test/scala/firrtlTests/VerilogEmitterTests.scala
@@ -420,27 +420,39 @@ class VerilogEmitterSpec extends FirrtlFlatSpec {
          |    input always: UInt<1>
          |    output always$: UInt<1>
          |    inst assign of endmodule
+         |    inst edge of endmodule_
          |    node always_ = not(always)
          |    node always__ = and(always_, assign.fork)
-         |    always$ <= always__
+         |    node always___ = and(always__, edge.fork)
+         |    always$ <= always___
          |  module endmodule:
          |    output fork: UInt<1>
          |    node const = add(UInt<4>("h1"), UInt<3>("h2"))
          |    fork <= const
+         |  module endmodule_:
+         |    output fork: UInt<1>
+         |    node const = add(UInt<4>("h1"), UInt<3>("h1"))
+         |    fork <= const
          |""".stripMargin
     val check_firrtl =
       """|circuit parameter_:
          |  module parameter_:
-         |    input always___: UInt<1>
+         |    input always____: UInt<1>
          |    output always$: UInt<1>
-         |    inst assign_ of endmodule_
-         |    node always_ = not(always___)
+         |    inst assign_ of endmodule__
+         |    inst edge_ of endmodule_
+         |    node always_ = not(always____)
          |    node always__ = and(always_, assign_.fork_)
-         |    always$ <= always__
-         |  module endmodule_:
+         |    node always___ = and(always__, edge_.fork_)
+         |    always$ <= always___
+         |  module endmodule__:
          |    output fork_: UInt<1>
          |    node const_ = add(UInt<4>("h1"), UInt<3>("h2"))
          |    fork_ <= const_
+         |  module endmodule_:
+         |    output fork_: UInt<1>
+         |    node const_ = add(UInt<4>("h1"), UInt<3>("h1"))
+         |    fork_ <= const_
          |""".stripMargin
     val state = CircuitState(parse(input), UnknownForm, Seq.empty, None)
     val output = Seq( ToWorkingIR, ResolveKinds, InferTypes, new VerilogRename )
diff --git a/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala b/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala
new file mode 100644
index 00000000..e0053fa8
--- /dev/null
+++ b/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala
@@ -0,0 +1,168 @@
+// See LICENSE for license details.
+
+package firrtlTests.features
+
+import firrtl.{ir, CircuitState, Parser, WDefInstance, WRef, WSubField}
+import firrtl.annotations.{CircuitTarget, IsMember, SingleTargetAnnotation}
+import firrtl.features.{LowerCaseNames, UpperCaseNames}
+import firrtl.options.Dependency
+import firrtl.transforms.ManipulateNamesBlocklistAnnotation
+import firrtl.testutils.FirrtlCheckers._
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+class LetterCaseTransformSpec extends AnyFlatSpec with Matchers {
+
+  case class TrackingAnnotation(val target: IsMember) extends SingleTargetAnnotation[IsMember] {
+    override def duplicate(a: IsMember) = this.copy(target=a)
+  }
+
+  class CircuitFixture {
+    private val input =
+      """|circuit Foo:
+         |  module Bar:
+         |    output OuT: UInt<2>
+         |    OuT <= UInt<2>(0)
+         |  module Baz:
+         |    output OuT: UInt<2>
+         |    OuT <= UInt<2>(1)
+         |  module baz:
+         |    output OuT: UInt<2>
+         |    OuT <= UInt<2>(2)
+         |  extmodule Ext:
+         |    output OuT: UInt<1>
+         |  module Foo:
+         |    input CLk: Clock
+         |    input rst_P: UInt<1>
+         |    input addr: UInt<8>
+         |    node Bar = UInt<1>(0)
+         |    reg baz: UInt<1>, CLk with: (reset => (rst_P, Bar))
+         |    wire QUX: UInt<1>
+         |    QUX <= UInt<1>(0)
+         |    node quuxQuux = UInt<1>(0)
+         |    mem MeM : @[Source.scala 1:4]
+         |      data-type => UInt<8>
+         |      depth => 32
+         |      read-latency => 0
+         |      write-latency => 1
+         |      reader => Read
+         |      writer => wRITE
+         |      readwriter => rw
+         |      read-under-write => undefined
+         |    MeM.Read is invalid
+         |    MeM.wRITE is invalid
+         |    MeM.rw is invalid
+         |    inst SuB1 of Bar
+         |    inst SuB2 of Baz
+         |    inst SuB3 of baz
+         |    inst SuB4 of Ext
+         |    node sub1 = UInt<1>(0)
+         |    node corge_corge = SuB1.OuT
+         |    node QuuzQuuz = and(SuB2.OuT, SuB3.OuT)
+         |    node graultGrault = not(SuB4.OuT)
+         |""".stripMargin
+
+    private val Foo = CircuitTarget("Foo")
+    private val Bar = Foo.module("Bar")
+
+    val annotations = Seq(TrackingAnnotation(Foo.module("Foo").ref("MeM").field("wRITE")field("en")),
+                          ManipulateNamesBlocklistAnnotation(Seq(Seq(Bar)), Dependency[LowerCaseNames]))
+    val state = CircuitState(Parser.parse(input), annotations)
+  }
+
+  behavior of "LowerCaseNames"
+
+  it should "change all names to lowercase" in new CircuitFixture {
+    val tm = new firrtl.stage.transforms.Compiler(Seq(firrtl.options.Dependency[LowerCaseNames]))
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "foo") => true },
+      { case ir.Module(_, "foo",
+                       Seq(ir.Port(_, "clk", _, _), ir.Port(_, "rst_p", _, _), ir.Port(_, "addr", _, _)), _) => true },
+      /* Module "Bar" should be skipped via a ManipulateNamesBlocklistAnnotation */
+      { case ir.Module(_, "Bar", Seq(ir.Port(_, "out", _, _)), _) => true },
+      { case ir.Module(_, "baz_0", Seq(ir.Port(_, "out", _, _)), _) => true },
+      { case ir.Module(_, "baz", Seq(ir.Port(_, "out", _, _)), _) => true },
+      /* External module "Ext" is not renamed */
+      { case ir.ExtModule(_, "Ext", Seq(ir.Port(_, "OuT", _, _)), _, _) => true },
+      { case ir.DefNode(_, "bar", _) => true },
+      { case ir.DefRegister(_, "baz", _, WRef("clk", _, _, _), WRef("rst_p", _, _, _), WRef("bar", _, _, _)) => true },
+      { case ir.DefWire(_, "qux", _) => true },
+      { case ir.Connect(_, WRef("qux", _, _, _), _) => true },
+      { case ir.DefNode(_, "quuxquux", _) => true },
+      { case ir.DefMemory(_, "mem", _, _, _, _, Seq("read"), Seq("write"), Seq("rw"), _) => true },
+      /* Ports of memories should be ignored, but these are already lower case */
+      { case ir.IsInvalid(_, WSubField(WSubField(WRef("mem", _, _, _), "read", _, _), "addr", _, _)) => true },
+      { case ir.IsInvalid(_, WSubField(WSubField(WRef("mem", _, _, _), "write", _, _), "addr", _, _)) => true },
+      { case ir.IsInvalid(_, WSubField(WSubField(WRef("mem", _, _, _), "rw", _, _), "addr", _, _)) => true },
+      /* Module "Bar" was skipped via a ManipulateNamesBlocklistAnnotation. The instance "SuB1" is renamed to "sub1_0"
+       * because node "sub1" already exists. This differs from the upper case test.
+       */
+      { case WDefInstance(_, "sub1_0", "Bar", _) => true },
+      { case WDefInstance(_, "sub2", "baz_0", _) => true },
+      { case WDefInstance(_, "sub3", "baz", _) => true },
+      /* External module instance names are renamed */
+      { case WDefInstance(_, "sub4", "Ext", _) => true },
+      { case ir.DefNode(_, "sub1", _) => true },
+      { case ir.DefNode(_, "corge_corge", WSubField(WRef("sub1_0", _, _, _), "out", _, _)) => true },
+      { case ir.DefNode(_, "quuzquuz",
+                        ir.DoPrim(_,Seq(WSubField(WRef("sub2", _, _, _), "out", _, _),
+                                        WSubField(WRef("sub3", _, _, _), "out", _, _)), _, _)) => true },
+      /* References to external module ports are not renamed, e.g., OuT */
+      { case ir.DefNode(_, "graultgrault",
+                        ir.DoPrim(_, Seq(WSubField(WRef("sub4", _, _, _), "OuT", _, _)), _, _)) => true }
+    )
+    expected.foreach( statex should containTree (_) )
+  }
+
+  behavior of "UpperCaseNames"
+
+  it should "change all names to uppercase" in new CircuitFixture {
+    val tm = new firrtl.stage.transforms.Compiler(Seq(firrtl.options.Dependency[UpperCaseNames]))
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "FOO") => true },
+      { case ir.Module(_, "FOO",
+                       Seq(ir.Port(_, "CLK", _, _), ir.Port(_, "RST_P", _, _), ir.Port(_, "ADDR", _, _)), _) => true },
+      /* Module "Bar" should be skipped via a ManipulateNamesBlocklistAnnotation */
+      { case ir.Module(_, "Bar", Seq(ir.Port(_, "OUT", _, _)), _) => true },
+      { case ir.Module(_, "BAZ", Seq(ir.Port(_, "OUT", _, _)), _) => true },
+      { case ir.Module(_, "BAZ_0", Seq(ir.Port(_, "OUT", _, _)), _) => true },
+      /* External module "Ext" is not renamed */
+      { case ir.ExtModule(_, "Ext", Seq(ir.Port(_, "OuT", _, _)), _, _) => true },
+      { case ir.DefNode(_, "BAR", _) => true },
+      { case ir.DefRegister(_, "BAZ", _, WRef("CLK", _, _, _), WRef("RST_P", _, _, _), WRef("BAR", _, _, _)) => true },
+      { case ir.DefWire(_, "QUX", _) => true },
+      { case ir.Connect(_, WRef("QUX", _, _, _), _) => true },
+      { case ir.DefNode(_, "QUUXQUUX", _) => true },
+      { case ir.DefMemory(_, "MEM", _, _, _, _, Seq("READ"), Seq("WRITE"), Seq("RW"), _) => true },
+      /* Ports of memories should be ignored while readers/writers are renamed, e.g., "Read" is converted to upper case
+       *  while "addr" is not touched.
+       */
+      { case ir.IsInvalid(_, WSubField(WSubField(WRef("MEM", _, _, _), "READ", _, _), "addr", _, _)) => true },
+      { case ir.IsInvalid(_, WSubField(WSubField(WRef("MEM", _, _, _), "WRITE", _, _), "addr", _, _)) => true },
+      { case ir.IsInvalid(_, WSubField(WSubField(WRef("MEM", _, _, _), "RW", _, _), "addr", _, _)) => true },
+      /* Module "Bar" was skipped via a ManipulateNamesBlocklistAnnotation. The instance "SuB1" is renamed to "SUB1"
+       * because this statement occurs before the "sub1" node later. This differs from the lower case test.
+       */
+      { case WDefInstance(_, "SUB1", "Bar", _) => true },
+      /* Instance "SuB2" and "SuB3" switch their modules from the lower case test due to namespace behavior. */
+      { case WDefInstance(_, "SUB2", "BAZ", _) => true },
+      { case WDefInstance(_, "SUB3", "BAZ_0", _) => true },
+      /* External module "Ext" was skipped via a ManipulateBlocklistAnnotation */
+      { case WDefInstance(_, "SUB4", "Ext", _) => true },
+      /* Node "sub1" becomes "SUB1_0" because instance "SuB1" already got the "SUB1" name. */
+      { case ir.DefNode(_, "SUB1_0", _) => true },
+      { case ir.DefNode(_, "CORGE_CORGE", WSubField(WRef("SUB1", _, _, _), "OUT", _, _)) => true },
+      { case ir.DefNode(_, "QUUZQUUZ",
+                        ir.DoPrim(_,Seq(WSubField(WRef("SUB2", _, _, _), "OUT", _, _),
+                                        WSubField(WRef("SUB3", _, _, _), "OUT", _, _)), _, _)) => true },
+      /* References to external module ports are not renamed, e.g., "OuT" */
+      { case ir.DefNode(_, "GRAULTGRAULT",
+                        ir.DoPrim(_, Seq(WSubField(WRef("SUB4", _, _, _), "OuT", _, _)), _, _)) => true }
+    )
+    expected.foreach( statex should containTree (_) )
+  }
+
+}
diff --git a/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala b/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala
new file mode 100644
index 00000000..b1e2eeb9
--- /dev/null
+++ b/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala
@@ -0,0 +1,229 @@
+// See LICENSE for license details.
+
+package firrtlTests.transforms
+
+import firrtl.{
+  ir,
+  CircuitState,
+  FirrtlUserException,
+  Namespace,
+  Parser,
+  RenameMap
+}
+import firrtl.annotations.CircuitTarget
+import firrtl.options.Dependency
+import firrtl.testutils.FirrtlCheckers._
+import firrtl.transforms.{
+  ManipulateNames,
+  ManipulateNamesBlocklistAnnotation,
+  ManipulateNamesAllowlistAnnotation,
+  ManipulateNamesAllowlistResultAnnotation
+}
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+object ManipulateNamesSpec {
+
+  class AddPrefix extends ManipulateNames {
+    override def manipulate = (a: String, b: Namespace) => Some(b.newName("prefix_" + a))
+  }
+
+}
+
+class ManipulateNamesSpec extends AnyFlatSpec with Matchers {
+
+  import ManipulateNamesSpec._
+
+  class CircuitFixture {
+    protected val input =
+      """|circuit Foo:
+         |  module Bar:
+         |    node a = UInt<1>(0)
+         |  module Foo:
+         |    inst bar of Bar
+         |    inst bar2 of Bar
+         |""".stripMargin
+    val `~Foo` = CircuitTarget("Foo")
+    val `~Foo|Foo` = `~Foo`.module("Foo")
+    val `~Foo|Foo/bar:Bar` = `~Foo|Foo`.instOf("bar", "Bar")
+    val `~Foo|Foo/bar2:Bar` = `~Foo|Foo`.instOf("bar2", "Bar")
+    val `~Foo|Bar` = `~Foo`.module("Bar")
+    val `~Foo|Bar>a` = `~Foo|Bar`.ref("a")
+    val tm = new firrtl.stage.transforms.Compiler(Seq(Dependency[AddPrefix]))
+  }
+
+  behavior of "ManipulateNames"
+
+  it should "rename everything by default" in new CircuitFixture {
+    val state = CircuitState(Parser.parse(input), Seq.empty)
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "prefix_Foo") => true },
+      { case ir.Module(_, "prefix_Foo", _, _) => true},
+      { case ir.Module(_, "prefix_Bar", _, _) => true}
+    )
+    expected.foreach(statex should containTree (_))
+  }
+
+  it should "do nothing if the circuit is blocklisted" in new CircuitFixture {
+    val annotations = Seq(ManipulateNamesBlocklistAnnotation(Seq(Seq(`~Foo`)), Dependency[AddPrefix]))
+    val state = CircuitState(Parser.parse(input), annotations)
+    val statex = tm.execute(state)
+    state.circuit.serialize should be (statex.circuit.serialize)
+  }
+
+  it should "not rename the circuit if the top module is blocklisted" in new CircuitFixture {
+    val annotations = Seq(ManipulateNamesBlocklistAnnotation(Seq(Seq(`~Foo|Foo`)), Dependency[AddPrefix]))
+    val state = CircuitState(Parser.parse(input), annotations)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "Foo") => true },
+      { case ir.Module(_, "Foo", _, _) => true},
+      { case ir.Module(_, "prefix_Bar", _, _) => true}
+    )
+    val statex = tm.execute(state)
+    expected.foreach(statex should containTree (_))
+  }
+
+  it should "not rename instances if blocklisted" in new CircuitFixture {
+    val annotations = Seq(ManipulateNamesBlocklistAnnotation(Seq(Seq(`~Foo|Foo/bar:Bar`)), Dependency[AddPrefix]))
+    val state = CircuitState(Parser.parse(input), annotations)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.DefInstance(_, "bar", "prefix_Bar", _) => true},
+      { case ir.Module(_, "prefix_Bar", _, _) => true}
+    )
+    val statex = tm.execute(state)
+    expected.foreach(statex should containTree (_))
+  }
+
+  it  should "do nothing if the circuit is not allowlisted" in new CircuitFixture {
+    val annotations = Seq(
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo|Foo`)), Dependency[AddPrefix])
+    )
+    val state = CircuitState(Parser.parse(input), annotations)
+    val statex = tm.execute(state)
+    state.circuit.serialize should be (statex.circuit.serialize)
+  }
+
+  it should "rename only the circuit if allowlisted" in new CircuitFixture {
+    val annotations = Seq(
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo`)), Dependency[AddPrefix]),
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo|Foo`)), Dependency[AddPrefix])
+    )
+    val state = CircuitState(Parser.parse(input), annotations)
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "prefix_Foo") => true },
+      { case ir.Module(_, "prefix_Foo", _, _) => true},
+      { case ir.DefInstance(_, "bar", "Bar", _) => true},
+      { case ir.DefInstance(_, "bar2", "Bar", _) => true},
+      { case ir.Module(_, "Bar", _, _) => true},
+      { case ir.DefNode(_, "a", _) => true}
+    )
+    expected.foreach(statex should containTree (_))
+  }
+
+  it should "rename an instance via allowlisting" in new CircuitFixture {
+    val annotations = Seq(
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo`)), Dependency[AddPrefix]),
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo|Foo/bar:Bar`)), Dependency[AddPrefix])
+    )
+    val state = CircuitState(Parser.parse(input), annotations)
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "Foo") => true },
+      { case ir.Module(_, "Foo", _, _) => true},
+      { case ir.DefInstance(_, "prefix_bar", "Bar", _) => true},
+      { case ir.DefInstance(_, "bar2", "Bar", _) => true},
+      { case ir.Module(_, "Bar", _, _) => true},
+      { case ir.DefNode(_, "a", _) => true}
+    )
+    expected.foreach(statex should containTree (_))
+  }
+
+  it should "rename a node via allowlisting" in new CircuitFixture {
+    val annotations = Seq(
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo`)), Dependency[AddPrefix]),
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo|Bar>a`)), Dependency[AddPrefix])
+    )
+    val state = CircuitState(Parser.parse(input), annotations)
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "Foo") => true },
+      { case ir.Module(_, "Foo", _, _) => true},
+      { case ir.DefInstance(_, "bar", "Bar", _) => true},
+      { case ir.DefInstance(_, "bar2", "Bar", _) => true},
+      { case ir.Module(_, "Bar", _, _) => true},
+      { case ir.DefNode(_, "prefix_a", _) => true}
+    )
+    expected.foreach(statex should containTree (_))
+  }
+
+  it should "throw user errors on circuits that haven't been run through LowerTypes" in {
+    val input =
+      """|circuit Foo:
+         |  module Foo:
+         |    wire bar: {a: UInt<1>, b: UInt<1>}
+         |    node baz = bar.a
+         |""".stripMargin
+    val state = CircuitState(Parser.parse(input), Seq.empty)
+    intercept [FirrtlUserException] {
+      (new AddPrefix).transform(state)
+    }.getMessage should include ("LowerTypes")
+  }
+
+  behavior of "ManipulateNamesBlocklistAnnotation"
+
+  it should "throw an exception if a non-local target is skipped" in new CircuitFixture {
+    val barA = CircuitTarget("Foo").module("Foo").instOf("bar", "Bar").ref("a")
+    assertThrows[java.lang.IllegalArgumentException]{
+      Seq(ManipulateNamesBlocklistAnnotation(Seq(Seq(barA)), Dependency[AddPrefix]))
+    }
+  }
+
+  behavior of "ManipulateNamesAllowlistResultAnnotation"
+
+  it should "delete itself if the new target is deleted" in {
+    val `~Foo|Bar` = CircuitTarget("Foo").module("Bar")
+    val `~Foo|prefix_Bar` = CircuitTarget("Foo").module("prefix_Bar")
+
+    val a = ManipulateNamesAllowlistResultAnnotation(
+      targets = Seq(Seq(`~Foo|prefix_Bar`)),
+      transform = Dependency[AddPrefix],
+      oldTargets = Seq(Seq(`~Foo|Bar`))
+    )
+
+    val r = RenameMap()
+    r.delete(`~Foo|prefix_Bar`)
+
+    a.update(r) should be (empty)
+  }
+
+  it should "drop a deleted target" in {
+    val `~Foo|Bar` = CircuitTarget("Foo").module("Bar")
+    val `~Foo|prefix_Bar` = CircuitTarget("Foo").module("prefix_Bar")
+    val `~Foo|Baz` = CircuitTarget("Foo").module("Baz")
+    val `~Foo|prefix_Baz` = CircuitTarget("Foo").module("prefix_Baz")
+
+    val a = ManipulateNamesAllowlistResultAnnotation(
+      targets = Seq(Seq(`~Foo|prefix_Bar`), Seq(`~Foo|prefix_Baz`)),
+      transform = Dependency[AddPrefix],
+      oldTargets = Seq(Seq(`~Foo|Bar`), Seq(`~Foo|Baz`))
+    )
+
+    val r = RenameMap()
+    r.delete(`~Foo|prefix_Bar`)
+
+    val ax = a.update(r).collect {
+      case b: ManipulateNamesAllowlistResultAnnotation[_] => b
+    }
+
+    ax should not be length (1)
+
+    val keys = ax.head.toRenameMap.getUnderlying.keys
+
+    keys should not contain (`~Foo|Bar`)
+    keys should contain (`~Foo|Baz`)
+  }
+
+}