Fix naming for RHS of named unapply expressions (backport #3163) (#3165)

* Fix naming for RHS of named unapply expressions (#3163)

Previously, the naming plugin would not recurse for the RHS of an
unapply, meaning that vals declared in blocks that ultimately return
some result that is unapplied into some named Data would not be named.

Also, audit and improve the naming mdocs.

(cherry picked from commit 262caa7)

# Conflicts:
#	docs/src/cookbooks/naming.md

* Resolve backport conflicts

---------

Co-authored-by: Jack Koenig <[email protected]>

docs/src/cookbooks/naming.md

@@ -7,6 +7,10 @@ section: "chisel3"
 ```scala mdoc:invisible
 import chisel3._
 import circt.stage.ChiselStage
+def emitSystemVerilog(gen: => RawModule): String = {
+  val prettyArgs = Array("--disable-all-randomization", "--strip-debug-info")
+  ChiselStage.emitSystemVerilog(gen, firtoolOpts = prettyArgs)
+}
 ```
 # Naming Cookbook
 
@@ -28,8 +32,8 @@ We recommend you manually insert calls to `prefix` to clarify these cases:
 import chisel3.experimental.prefix
 class ExamplePrefix extends Module {
 
-  Seq.tabulate{2} {i =>
-    Seq.tabulate{2}{ j =>
+  Seq.tabulate(2) { i =>
+    Seq.tabulate(2) { j =>
       prefix(s"loop_${i}_${j}"){
         val x = WireInit((i*0x10+j).U(8.W))
         dontTouch(x)
@@ -39,7 +43,7 @@ class ExamplePrefix extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new ExamplePrefix)
+emitSystemVerilog(new ExamplePrefix)
 ```
 ### How can I get better names for code generated by `when` clauses?
 
@@ -53,18 +57,20 @@ class ExampleWhenPrefix extends Module {
 
   out := DontCare
 
-  Seq.tabulate{2}{ i =>
+  Seq.tabulate(2) { i =>
     val j = i + 1
-    when (in === j.U) { prefix(s"clause_${j}"){
-      val foo = Wire(UInt(4.W))
-      foo := in +& j.U(4.W)
-      out := foo
-    }}
+    prefix(s"clause_${j}") {
+      when (in === j.U) {
+        val foo = Reg(UInt(4.W))
+        foo := in + j.U(4.W)
+        out := foo
+      }
+    }
   }
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new ExampleWhenPrefix)
+emitSystemVerilog(new ExampleWhenPrefix)
 ```
 
 ### I still see _GEN signals, can this be fixed?
@@ -100,13 +106,17 @@ class ExampleNoPrefix extends Module {
   val in = IO(Input(UInt(2.W)))
   val out = IO(Output(UInt()))
 
-  val add = noPrefix { in + in + in }
+  val add = noPrefix {
+    // foo will not get a prefix
+    val foo = RegNext(in + 1.U)
+    foo + in
+  }
 
   out := add
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new ExampleNoPrefix)
+emitSystemVerilog(new ExampleNoPrefix)
 ```
 
 ### I am still not getting the name I want. For example, inlining an instance changes my name!
@@ -138,7 +148,7 @@ class MyLeaf extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new WrapperExample)
+emitSystemVerilog(new WrapperExample)
 ```
 
 This can be used to rename instances and non-aggregate typed signals.

docs/src/explanations/naming.md

@@ -21,7 +21,14 @@ systemic name-stability issues, please refer to the naming [cookbook](../cookboo
 // Imports used by the following examples
 import chisel3._
 import chisel3.experimental.{prefix, noPrefix}
+```
+
+```scala mdoc:invisible
 import circt.stage.ChiselStage
+def emitSystemVerilog(gen: => RawModule): String = {
+  val prettyArgs = Array("--disable-all-randomization", "--strip-debug-info")
+  ChiselStage.emitSystemVerilog(gen, firtoolOpts = prettyArgs)
+}
 ```
 
 With the release of Chisel 3.5, users are required to add the following line to
@@ -51,7 +58,7 @@ class Example1 extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example1)
+emitSystemVerilog(new Example1)
 ```
 
 Otherwise, it is rewritten to also include the name as a prefix to any signals generated while executing the right-hand-
@@ -75,7 +82,7 @@ class Example2 extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example2)
+emitSystemVerilog(new Example2)
 ```
 
 Prefixing can also be derived from the name of signals on the left-hand side of a connection.
@@ -98,7 +105,7 @@ class ConnectPrefixing extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new ConnectPrefixing)
+emitSystemVerilog(new ConnectPrefixing)
 ```
 
 Note that the naming also works if the hardware type is nested in an `Option` or a subtype of `Iterable`:
@@ -109,19 +116,22 @@ class Example3 extends Module {
   // val in = autoNameRecursively("in")(prefix("in")(IO(Input(UInt(2.W)))))
 
   val out = IO(Output(UInt()))
-  // val out = autoNameRecursively("out")(prefix("out")(IO(Output(UInt(2.W)))))
+  // val out = autoNameRecursively("out")(prefix("out")(IO(Output(UInt()))))
 
-  def inXin() = in * in
+  def func() = {
+    val delay = RegNext(in)
+    delay + 1.U
+  }
 
-  val opt = Some(3.U + inXin())
-  // Note that the intermediate result of the inXin() is prefixed with `opt`:
-  // val opt = autoNameRecursively("opt")(prefix("opt")(Some(3.U + inXin())))
+  val opt = Some(func())
+  // Note that the register in func() is prefixed with `opt`:
+  // val opt = autoNameRecursively("opt")(prefix("opt")(Some(func()))
 
   out := opt.get + 1.U
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example3)
+emitSystemVerilog(new Example3)
 ```
 
 There is also a slight variant (`autoNameRecursivelyProduct`) for naming hardware with names provided by an unapply:
@@ -135,7 +145,7 @@ class UnapplyExample extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new UnapplyExample)
+emitSystemVerilog(new UnapplyExample)
 ```
 
 Note that the compiler plugin will not insert a prefix in these cases because it is ambiguous what the prefix should be.
@@ -169,8 +179,8 @@ class Example5 extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example4)
-ChiselStage.emitSystemVerilog(new Example5)
+emitSystemVerilog(new Example4)
+emitSystemVerilog(new Example5)
 ```
 
 Also note that the prefixes append to each other (including the prefix generated by the compiler plugin):
@@ -186,7 +196,7 @@ class Example6 extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example6)
+emitSystemVerilog(new Example6)
 ```
 
 Sometimes you may want to disable the prefixing. This might occur if you are writing a library function and
@@ -203,7 +213,7 @@ class Example7 extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example7)
+emitSystemVerilog(new Example7)
 ```
 
 ### Suggest a Signal's Name (or the instance name of a Module)
@@ -222,7 +232,7 @@ class Example8 extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example8)
+emitSystemVerilog(new Example8)
 ```
 
 Note that using `.suggestName` does **not** affect prefixes derived from val names;
@@ -265,7 +275,7 @@ class ConnectionPrefixExample extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new ConnectionPrefixExample)
+emitSystemVerilog(new ConnectionPrefixExample)
 ```
 
 As this example illustrates, this behavior is slightly inconsistent so is subject to change in a future version of Chisel.
@@ -291,7 +301,7 @@ class TemporaryExample extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new TemporaryExample)
+emitSystemVerilog(new TemporaryExample)
 ```
 
 If an unnamed signal is itself used to generate a prefix, the leading `_` will be ignored to avoid double `__` in the names of further nested signals.
@@ -311,7 +321,7 @@ class TemporaryPrefixExample extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new TemporaryPrefixExample)
+emitSystemVerilog(new TemporaryPrefixExample)
 ```
 
 
@@ -333,8 +343,8 @@ class Example9(width: Int) extends Module {
 }
 ```
 ```scala mdoc:verilog
-ChiselStage.emitSystemVerilog(new Example9(8))
-ChiselStage.emitSystemVerilog(new Example9(1))
+emitSystemVerilog(new Example9(8))
+emitSystemVerilog(new Example9(1))
 ```
 
 ### Reflection Naming

plugin/src/main/scala/chisel3/internal/plugin/ChiselComponent.scala

@@ -233,7 +233,8 @@ class ChiselComponent(val global: Global, arguments: ChiselPluginArguments)
             case Some(names) =>
               val onames: List[Option[String]] =
                 fieldsOfInterest.zip(names).map { case (ok, name) => if (ok) Some(name) else None }
-              val named = q"chisel3.internal.plugin.autoNameRecursivelyProduct($onames)($rhs)"
+              val newRHS = transform(rhs)
+              val named = q"chisel3.internal.plugin.autoNameRecursivelyProduct($onames)($newRHS)"
               treeCopy.ValDef(dd, mods, name, tpt, localTyper.typed(named))
             case None => // It's not clear how this could happen but we don't want to crash
               super.transform(tree)

src/test/scala/chiselTests/naming/NamePluginSpec.scala

@@ -267,6 +267,22 @@ class NamePluginSpec extends ChiselFlatSpec with Utils {
     }
   }
 
+  "Unapply assignments" should "name (but not prefix) local vals on the RHS" in {
+    class Test extends Module {
+      {
+        val (a, b) = {
+          val x, y = Wire(UInt(3.W))
+          val sum = WireInit(x + y)
+          (x, y)
+        }
+      }
+    }
+
+    aspectTest(() => new Test) { top: Test =>
+      Select.wires(top).map(_.instanceName) should be(List("a", "b", "sum"))
+    }
+  }
+
   "Unapply assignments" should "not override already named things" in {
     class Test extends Module {
       {