[RFC] What does it mean to be a Bundle?

[jackkoenig]

The standard pattern for using Chisel Data objects as generator parameters in Bundles is to pass them as a constructor argument like so:

class MyBundle[T <: Data](gen: T) extends Bundle {
  val foo = Input(gen)
}

In the modern world of autoclonetype; however, you are encouraged to make constructor arguments vals so that reflection can find them. If you make gen a val:

class MyBundle[T <: Data](val gen: T) extends Bundle {
  val foo = Input(gen)
}

Whoops, you've accidentally added a new element to your Bundle called gen! This is due to the fact that the elements of a Bundle are defined (by implementation) to be the public vals of type Data or Some[Data].

Instead, you should use a private val:

class MyBundle[T <: Data](private val gen: T) extends Bundle {
  val foo = Input(gen)
}

While a little verbose this allows autoclonetype to automatically generate clonetype so everyone is happy.

This led to a discussion on what the fundamental definition of a Bundle is/should be. As far as I can tell there are two possibilities:

1. Bundle Elements are the public vals of the Scala class (the status quo)

As far as I know, this hasn't been much a point of confusion for new users; however, with autoclonetype encouraging people to make their constructor parameters vals it most certainly could become one. To make this better, I think there are a couple of things we should do:

1. Write a wiki page on the definition of a Bundle with examples
2. When a chisel3.core.Binding$MixedDirectionAggregateException caused by a val constructor parameter of type Data or Some[Data], inform the user that it counts as an element and link to the wiki page.
3. Augment autoclonetype to detect elements that are also constructor parameters and implicitly call cloneType on them. This is needed since these constructor arguments are not just generator parameters, they belong to the Bundle so we need fresh objects.
   • Added bonus of this is it should enable single-line case class Bundles

There is one case where I'm not sure if we'll be able to [easily] provide a decent error message though. For a Bundle with no directions at all, you can end up with a firrtl.passes.CheckInitialization$RefNotInitializedException if you have a val constructor parameter of type Data that you didn't intend to be an element of the Bundle.

2. Bundle Elements should be vals in the body of a Bundle

It is a keen observation that there are very few examples of people using val constructor parameters to create fields of Bundles (I only know of one h/t @grebe). Perhaps it is better to redefine Bundles such that constructor arguments of type Data or Some[Data] are not elements, rather they are just constructor parameters.

This has the nice property of encouraging separation of generator parameters and elements of the Bundle, and encourages people to express Bundles in a way similar to a C-struct. Misuse (eg. connecting to a val that is a constructor parameter) is also very easy to detect and provide a specific error message. As far as I know there aren't any obvious cases we can't catch like the one above. That being said this kind of a change would require carefully considering its implications on inheritance (especially overriding elements that may be a constructor parameters for a parent class)

I think there are other benefits as well, but I'm blanking so maybe @ducky64 can comment with some thoughts.

In any case, I think we should stuck with 1. but after some discussion in a meeting we thought we ought to ask the community to see what users think!

• Type of issue

  • Bug report
  • Feature request
  • Other enhancement

• What is the use case for changing the behavior?

See 2. above.

• Impact

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

• Development Phase

  • request
  • proposal

[ducky64]

I think the goal of Option 2 is more elegant and intuitive (visually obvious what is and isn't a Bundle field, compared to using a Scala keyword to double as "this is a Bundle field" and a Java access modifier to double as "no, I didn't really mean that to be a Bundle field"), but YMMV.

One issue with Option 2 is implementation, it would require reflection-based checking. Java reflection can only get constructor argument types, but not names. So in the case where you have a Data subtype in the argument list, the Bundle class must be Scala reflect-able. The only case I know of where this fails is for classes within an anonymous outer class, where reflection barfs with a CyclicReference exception. This is probably a Scala bug. (otherwise, most of the cases where autoclonetype fails isn't an issue here: it doesn't need the outer instance that autoclonetype does).

Reflection has also been slightly wobbly in general, such as the case of Class.forName in the previous version of Chisel's user-code-line locator for errors interacting badly with constructor invocations in clonetype. But that's mostly out of our control, probably a Java/Scala bug.

Aliasing is another issue with directly using constructor Data arguments. For example, one case that would appear correct is

class MyUselessBundle(val thisIsAField: Data) extends Bundle

class MyBundle(private val dataType: Data) extends Bundle {
  val field1 = new MyUselessBundle(dataType)
  val field2 = new MyUselessBundle(dataType)
}

MyBundle.field1.thisIsAField would reference alias with MyBundle.field2.thisIsAField (there needs to be a cloneType or some other API like Field(...) - see #758). And to have a cloneType or Field(...) wrapper, it needs to be in the body (I think?).

In general, proper style should probably recommend that leaf-level instantiations of a type be wrapped in cloneType or Field(...). We can catch these cases with a little work, and report as a runtime deprecation (though this can't trigger a compile-time warning).

tl;dr: I like option 2 style-wise and option 1 has a critical issue handling aliasing, but there are some implementation risks with option 2 that I think shouldn't affect the majority of use cases. In both cases Chisel shouldn't fail silently and give you wrong RTL, but giving helpful error messages in all cases for either option may not be straightforward.

[ducky64]

Resolution from today's meeting:

Immediate plan for 3.1

• Breaking things seriously sucks for users, so we're going to try and preserve current behavior, even if few users seem to be using it.
• For 3.1, Bundle field detection will continue under current rules (is it a val), and autoclonetype will clone data parameters. These types must still be unbound, bound types will trigger an autoclonetype error (because we cannot perfectly and meaningfully clone a bound type). The two options for constructor arguments of type Data that are not meant to be fields are:
  1. Make them private vals, which allows the use of autoclonetype. This is ugly.
  2. Define a custom cloneType, which is probably what most code was already doing.

Future cleaner APIs

For 3.1.1 or 3.2, we will be introducing a new, optional Bundle Field(...) API with better semantics, which will coexist with the old semantics indefinitely. The new semantics are, tentatively:

• Fields are all or nothing, if you define any Bundle element as a Field, you must define all Bundle elements as Fields. The compiler will error out if you either have vals of type Data that aren't declared as a Field, or if you have Fields that weren't autodetected to be Bundle fields (which is a limitation of reflection-based detection).
• As it's not possible to have a Field(...) declaration in a constructor argument, constructor Data vals will NOT be treated as fields.
• Field(...) will be a Bundle-specific API, and cannot be called outside a Bundle.
• This should eliminate all uses of cloneType in non-experimental Chisel constructs. In experimental, Record may still need cloneType, but Record is an advanced construct, and we may redefine it in the future to be more friendly.
• There was discussion about changing the name of this new API, for example as Struct (or something) instead of Bundle. I'm inclined to stick with Bundle, but there may be a discussion to be continued here.
• We will encourage the use of Field in new code, and there is no set timeline for when (if ever) old style code will generate deprecations, warnings, or errors.

Suggestions are welcome in designing a better 3.1.1/3.2 API. This is not yet set in stone and is not expected to produce compatibility issues with old code.