Refine autonaming to have more intuitive behavior

[jackkoenig]

Last name in an Expression wins, while the first Statement to name wins.
This is done via checking the _id of HasIds during autonaming and only
applying a name if the HasId was created in the scope of autonaming.
There is no change to .autoSeed or .suggestName behavior.

Behavior of chisel3-plugins from before this change is maintained. More precisely, assuming this PR is released with Chisel v3.4.1, if you use Chisel v3.4.1 with the v3.4.0 (or v3.4.0-RC3) plugin, you'll get the v3.4.0 behavior.

The bulk of the work here was making unapply continue to work because it previously worked via overriding names. Now, a signal will only be named by autoNameRecursively or autoNameRecursivelyProduct calls that wrap the Expression within which the Data is created.

Fixes #1603

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you state the API impact?
• Did you specify the code generation impact?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• bug fix
• new feature/API

API Impact

This changes the naming behavior to what I've been calling "Outer Expression, First Statement" wins. For example:

  val width: Option[Int] = Some(8)
  val debug = width.map { w =>
    val foo = IO(Output(UInt(w.W))) // name: "debug_foo"
    val delay = RegNext(in) // name: "debug_delay"
    foo := delay
    foo
  } // name: "debug"
  val bar = debug // name: "bar"

Intuitively, the name of this port should be debug. It is useful that things built within the anonymous function should have debug as a prefix in addition to their val name (eg. debug_delay), but because foo is returned, the outer name should win (debug). It is also intuitive that debug wins over bar since debug is the name of the declaration rather than an alias.

Prior to this PR, the port would be named debug_foo and if it were a non-port, it would be named bar. This PR unifies the behavior of ports and non-ports into what is (in my opinion) more intuitive.

Backend Code Generation Impact

Change in naming algorithm

Desired Merge Strategy

• Squash

Release Notes

Change naming priority to "Outer Expression, First Statement"

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[mwachs5]

👍 for the algorithm/behavior as documented in the comments

[jackkoenig]

Marked DO NOT MERGE because I need to write docs and I'm seeing a handful of cases of worse names that I wouldn't expect so I need to dig into it.

[jackkoenig]

I'm seeing a handful of cases of worse names that I wouldn't expect

In a randomish sampling, I found this behavior to be correct. In some cases a temporary name would get overridden to a better, non-temporary name; however, it could also result in overriding names in a bad way. For example:

class Example extends MultiIOModule {
  def extractBelow(from: UInt, idx: Int): UInt = {
    val x = from
    x(idx, 0)
  }
  
  val in  = IO(Input(UInt(16.W)))
  val out = IO(Output(UInt(16.W)))
  // If foo is a field of Example, reflective naming hides the bug
  if (true) {
    val foo = Reg(UInt(16.W))
    foo := in
    out := extractBelow(foo, 15)
  }
}

With v3.4.0 gives:

circuit Example :
  module Example :
    input clock : Clock
    input reset : UInt<1>
    input in : UInt<16>
    output out : UInt<16>
    reg out_x : UInt<16>, clock with :
      reset => (UInt<1>("h0"), out_x)
    out_x <= in
    node _out_T = bits(out_x, 15, 0)
    out <= _out_T

(Scastie: https://scastie.scala-lang.org/T6mdGw3LQOSNc2yEyUMy5w)

See how the val x in the utility function (with prefix out from the :=) overrides the name? In this case, it's clearly not desirable. That being said, in other cases, this change turns some good names into temporaries. For example, a slight modification on the above:

class Example extends MultiIOModule {
  def extractBelow(from: UInt, idx: Int): UInt = {
    val x = from
    x(idx, 0)
  }
  
  val in  = IO(Input(UInt(16.W)))
  val out = IO(Output(UInt(16.W)))
  // If foo is a field of Example, reflective naming hides the bug
  if (true) {
    val foo = Reg(UInt(16.W))
    foo := in
    out := extractBelow(foo +& 1.U, 15) // <---- difference here
  }
}

With v3.4.0 gives:

circuit Example :
  module Example :
    input clock : Clock
    input reset : UInt<1>
    input in : UInt<16>
    output out : UInt<16>
    reg foo : UInt<16>, clock with :
      reset => (UInt<1>("h0"), foo)
    foo <= in
    node out_x = add(foo, UInt<1>("h1"))
    node _out_T = bits(out_x, 15, 0)
    out <= _out_T

(Scastie: https://scastie.scala-lang.org/3FoJCFOmRUK5iYteYkkvjg)

In this case, it's nice that the sum gets the name out_x. With the change in this PR, instead you get:

circuit Example :
  module Example :
    input clock : Clock
    input reset : UInt<1>
    input in : UInt<16>
    output out : UInt<16>
    reg foo : UInt<16>, clock with :
      reset => (UInt<1>("h0"), foo)
    foo <= in
    node _out_T = add(foo, UInt<1>("h1"))
    node _out_T_1 = bits(_out_T, 15, 0)
    out <= _out_T_1

The loss of out_x is a bit unfortunate, but one could argue that this is more predicable since the Expression at the callsite does not have a name. If we prioritize predictability over "best possible names" then I think this is actually good.

[jackkoenig]

@azidar I've done an initial updating of the docs, but I have not yet added a description of "Outer Expression, First Statement". I would appreciate review of the code and tests while I finish up the docs.

[azidar]

Is the plan to remove autoseed in the future?

[jackkoenig]

Possibly, a good question for the dev meeting

[azidar]

Is this never empty?

[jackkoenig]

If none of the fields are <: Data then it will be empty, eg.

val (a, b) = ("hi", 3)