Avoid making unnecessary clones during binding

[zyedidia]

This change prevents unnecessary clones from happening where possible. Currently when creating a binding, the underlying Data object is always cloned. For example, elaborating this module creates 12 Foo objects.

class Foo extends Bundle {
  val a = UInt(8.W)
}
class MyModule extends RawModule {
  val io = IO(Flipped(new Bundle {
    val foo = Output(new Foo)
    val bar = Input(new Foo)
  }))
}

Since the objects being passed to the binding functions are never used again they do not need to be copied. After this PR, only 2 Foo objects are created when elaborating MyModule.

This change improves elaboration time and memory usage. On two large designs, I saw the following improvements:

Design 1

Metric	Default	Optimized cloning	Improvement
Elaboration time (s)	60.53	55.96	7.5%
Memory usage (MiB)	4389	4000	7%

Design 2

Metric	Default	Optimized cloning	Improvement
Elaboration time (s)	93.57	86.40	7.5%
Memory usage (MiB)	6180	5974	3%

The change works by evaluating the latest builder ID before and after evaluation of the bound expression. If it is newly created (created during the binding call itself), then it is assumed to never be used again (uncaptured), and can be safely reused without cloning.

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

• performance improvement

API Impact

This change is mostly source compatible, except in cases that should already be avoided. If the user creates a fresh type during binding, but also captures it into a variable at a higher scope, then it could be reused and doing so would cause errors after this change. This is a bad practice in general, so users should not be doing this in the first place.

For example, this program now causes an error because the result of mk() is not cloned and the binding is added directly to the object, but it is actually captured into x and used again (causing an error).

var x: UInt = null;

def mk(): UInt = {
  x = UInt(8.W)
  x
}

val foo = Wire(mk())
val bar = Wire(x)

Note: Scala 3 includes support for capture checking which would allow Chisel to ensure that the type has not been captured externally.

This change is not binary compatible because some public functions have changed signatures (bindings are now call-by-name instead of call-by-value).

Backend Code Generation Impact

No impact to code generation.

Desired Merge Strategy

Squash.

Release Notes

Bindings now use call-by-name to avoid unnecessary clones of data objects. This change does alter the behavior of programs that perform a capture to an outer scope during a binding (bad practice). Elaboration time and memory usage may improve by about 7%.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[jackkoenig]

Suggested change

	private var externalRef = memoVal[Boolean](None, () => elements.exists(_._2._id < _id))
	private[chisel3] def hasExternalRef(): Boolean = externalRef.get()
	lazy val hasExternalRef: Boolean = this.elements.exists(_._2._id < _id)

Scala actually has a built-in for this sort of thing 🙂 This should also use less memory because it only adds a 1 byte field (and uses 1 extra bit in the lazy val mask that already exists because we have other lazy vals in Bundles). Your approach adds a 4-byte field pointing to a 24-40 byte object. This might actually suggest rerunning the memory use check because 28-bytes per Bundle is non-trivial.

[jackkoenig]

Suggested change

	println(s"copies: ${Foo.count}")
	Foo.count should be < 12L
	Foo.count should be(2)

Also, it would be good to have a test that sharing a single Foo for the Input and Output does result in cloning.

class MyModule extends RawModule {
  val foo = new Foo
   val io = IO(Flipped(new Bundle {
     val foo = Output(foo)
     val bar = Input(foo)
   }))
 }
// This should have 3 Foos constructed

object Bar {
   var count = 0L
 }

 class Bar(x: UInt) extends Bundle {
   val a = x
   Bar += 1
 }
class MyModule extends RawModule {
   val io = IO(Flipped(new Bundle {
     val foo = Output(new Bar(UInt(8.W)))
     val bar = Input(new Bar(UInt(8.W)))
   }))
 }
// This should have 4 Bars constructed

Also a test that shows an extra clone when you pass a Data object to the Bundle that is used as a field (aka an externalRef).

[jackkoenig]

Suggested change

	lazy val hasExternalRef: Boolean = this.elements.exists(_._2._id < _id)
	private[chisel3] lazy val hasExternalRef: Boolean = this.elements.exists(_._2._id < _id)

[jackkoenig]

LGTM! Excellent work @zyedidia!