Autoclonetype will clone args that are of type data

[ducky64]

• Related issue (if applicable)
  Short-term (until 3.1.1/3.2) patch for [RFC] What does it mean to be a Bundle? #765 to work better with autoclonetype

This does not attempt to give an informative error message for MixedDirectionAggregateException , because the exception will not be raised in all cases (for example, if the Bundle contents were not directioned), it will require significant heuristic code to catch what seems to be a very uncommon edge case, sufficient debugging information is already printed, and an API addition for 3.1.1/3.2 should remove ambiguity.

Also makes ArbiterIO cloneable through a cloneType implementation.

• Type of change

  • Bug report
  • Feature request
  • Other enhancement

• Impact

  • no functional change
  • API addition (no impact on existing code)
  • API modification

• Development Phase

  • proposal
  • implementation

• Release Notes
  Autoclonetype will work with Bundle that have fields defined in their constructors.
  I personally don't think that's good style. as it raises the learning curve needed, but apparently it has users.
  See [RFC] What does it mean to be a Bundle? #765 for proposals for 3.1.1/3.2, where we may add an explicit (but optional) Field(...) construct to Bundle.

[jackkoenig]

I actually think we should change the constructor arguments to private val gen: T, val n: Int to take advantage of autoclonetype rather than implementing a clonetype.

I ran into an interesting issue trying to get all of this working in rocket-chip. If you override clonetype and then someone extends the class, you get runtime exceptions. This occurs in rocket-chip with QueueIO extended here.

I think we should change all of the library types to use autoclonetype

[ducky64]

Good points, converted to use autoclonetype. I don't like private vals (from my view of a new-user wtfs/min metric - which may or may not reflect reality), but I can see how clonetype + subclasses could cause even more unintuitive errors.
I've added a explanation about private val, because in the absence of knowledge about the details of autoclonetype or bundle rules, I would be going wtf at the code.
I haven't tested on rocket yet. Also, does #769 need to be merged first?

[kevintownsend]

Can you just link to the issue? (and just make this a one line comment) (1) It's easier to search for parts of the code related to the issue. (2) It's easier to get updates if someone sees this in the code.

[ducky64]

Good point, shortened the comment and linked the relevant issue.

[ducky64]

@jackkoenig making Decoupled use autoclonetype turns out to break rocket-chip, since there's a use of a bound gen...

[ducky64]

@ucbjrl test seems to be failing because something is causing sbt to go crazy, apparently wiping the coreMacros/target, chiselFrontend/target, and target folder should solve it.

[error] java.lang.AssertionError: assertion failed: Modified names for chisel3.util.DecoupledIO is empty
[error] 	at scala.Predef$.assert(Predef.scala:219)

[grebe]

I'd like to add that there are reasons you might not want gen to be private- e.g. we do matching on gen when generating IPXact for polymorphic bundles. This could all be structured in a way that deals with gen being private, but that seems like a pretty heavy constraint to work around something that is supposed to be making things easier.

Forgive me if previous discussions have raised this point, but couldn't we use the bindings system? If a val in a Bundle has no IO binding, just leave it out of the bundle. I suppose we could instead have an explicit hardware prototype binding that marks a chisel type as not belonging in a bundle. Some sort of PolymorphicBundle[T <: Data](val proto: T) extends Bundle { ... } could handle the binding for you.

[ucbjrl]

retest this please

[jackkoenig]

@grebe How do you access gen currently? The use of private vals as in this PR is the current behavior so I'm guessing you define accessors for the generator parameters?

I'm not sure how the bindings system could handle this, is your suggestion that if you instantiate PolymorphicBundle as:
new PolymorphicBundle(UInt(32.W)) then proto will not be a field where as new PolymorphicBundle(Input(UInt(32.W))) then proto will be a field of the bundle?
That could work but what about the situation where you don't want any directions in the Bundle?

[ducky64]

The issue with bindings is that Bundle fields aren't consistently bound. Some fields might have Input or Output bindings, but some might have none, for example

class MyBundle extends Bundle {
  val field = UInt(8.W)
}

(where field is an unbound Bundle field)
When a Bundle is wrapped in a binding operation (like IO(new MuBundle)), it relies on the Bundle's element detection to determine which are fields. So bindings can't be used to determine Bundle fields, unless we introduce some kind of Field(...) API for bundles, as being discussed in #765

[ucbjrl]

retest this please.

[grebe]

@jackkoenig I think right now we actually match on something that the prototype is used to make, i.e. if a field is val in = Flipped(Decoupled(gen)) we mach on in.bits. That's a fair point- it's not like there's a clean way to do this right now.

@ducky64 is it legal to have an unbound element in a Bundle? Shouldn't that be some sort of error?

[ducky64]

@grebe When you wrap the Bundle in IO(...) (or Reg(...), or etc), that binds it and its fields, using the field detection scheme mentioned above. Before that point, its elements do not need to be bound, though some of them may have directionality assigned with Input(...) or Output(...).

[jackkoenig]

retest this please

[jackkoenig]

It's failing because of the sbt dependency bug 🙄

[error] java.lang.AssertionError: assertion failed: Modified names for chisel3.util.DecoupledIO is empty
[error] 	at scala.Predef$.assert(Predef.scala:219)
[error] 	at sbt.internal.inc.NamesChange.<init>(Changes.scala:54)
[error] 	at sbt.internal.inc.IncrementalNameHashing.sameAPI(IncrementalNameHashing.scala:46)
[error] 	at sbt.internal.inc.IncrementalCommon.sameClass(IncrementalCommon.scala:205)
[error] 	at sbt.internal.inc.IncrementalCommon.$anonfun$changedIncremental$1(IncrementalCommon.scala:187)

[ucbjrl]

retest this please - after find ... -depth -type d -name target -exec rm -rf {} \;