The definitive binding refactor thread, request for comments

[ducky64]

This is a summary of all the issues related to the bindings refactor (issues #440, #445, #419, #399, #368, #282, parts of #535, #543), and the proposed path forward. Work on this should begin soon.

Terminology & Conventions

These will primarily be documentation clarifications.

• Terminally-bound nodes will be called hardware
• Non-terminally bound nodes will be called chisel types
• "conventional" (non-flipped) Bundle directionality is as a dataflow source or master (when relevant, for some buses).
  • This is consistent with current Decoupled convention.

Directionality

• Nodes can have these directions: Input, Output, Bidirectional (for Aggregates, below, or Analog), or Unspecified (default).
• Output(...) and Input(...) will (effectively) clear direction of internal nodes, so the top-level node can be set to an input/output.
• Aggregate rules, including Bundles and Vecs:
  • All or no elements must be Unspecified.
  • Considered Bidirectional if there are both Input, Output, or Bidirectional elements, otherwise either Input, Output, or Unspecified.
  • Can only contain chisel types
• Internally: Deduplicate the directionality data source by using only hierarchical directionality (dropping leaf directionality). Leaf directionality can be resolved using parent links.
  • Since top-level bindings are required, that becomes a convenient place to create parent links.
  • Chisel (compatibility mode) didn't require top-level bindings, but Chisel also skips the directionality system. firrtlDirection (hierarchical directionality) is sufficient to emit FIRRTL.
  • For performance reasons, leaf directionality can still be cached.
  • Bonus: this may eliminate the need for _onModuleClose
• Parent links will be a private chisel3 feature for now.

Binding operations

• Wire(t) must have t of chisel type
• Reg(t), Mem(t), Vec(t) must have t of chisel type that is not Bidirectional (either Input, Output, or Unspecified)
• Vec(t) additionally is defined to create a chisel type.
• IO(t): must have t of chisel type with direction.
• Hardware instantiation binding operations will return a new node (rather than binding directly to the input chisel type node), which I think is current behavior.
• Vec(args*), Vec(elts) deprecated, replace with VecWire(elts) (or similar, someone suggest good names). elts must be a hardware node, and this returns a hardware node.

Node operations

• .chiselCloneType and .cloneType are to be deprecated.
• .getType operator introduced, legal on hardware nodes.
• Do we want a .wipeDir operator? The main use case would be for specifying Mem type, but the same could be done by wrapping it in Input(...).
• Add .requireIsHardware, requireIsChiselType, .requireDirectioned, and .requireUndirectioned APIs.
• These new operations will not be available in compatibility mode, since they depend on parent links.
• Question: should these operators be as a method of the node (myNode.getType) or as a global method (getType(myNode))? The first is consistent with cloneType, but the second is consistent with the binding wrappers and Input(...) / Output(...)

Style recommendations

• Methods parameterized by chisel type (usually called gen) should take a chisel type only, and should requireIsChiselType (or requireDirectioned / requireUndirectioned to sanity-check their inputs. Use of cloneType / getType in these occasions is discouraged and should be the responsibility of the caller.
• Bundles used as structs (as opposed to IOs) should not be directioned, allowing them to be directly used inside Reg/Mem. Directioning should generally happen "late", like when necessary (either for IO or for truly bidirectional structs).

[ducky64]

So there may be one issue with the direction refactor: the new semantics require explicit direction somewhere in the chain for ports, while Chisel (compatibility) assumed that nodes without direction were Outputs and nodes with just flip are Inputs. This becomes a problem when using Chisel._ bundles with the old semantics in a chisel3._ Module. Possible solutions:

• Just error out with a chisel3 style error, and users must make their Chisel._ bundles compliant with chisel3._ semantics.
• Put compatibility checking code in chisel3. This is going to be ugly and hacky. The general philosophy recently has been to move more and more code into Chisel._, so chisel3.core is as clean as possible.
• Give up the new strict directionality semantics. It seems that a lot of chisel3 code already follows the new, stricter rules, just that we haven't been checking it.

This case came up in CompatibilityInteroperabilitySpec.scala, with this old-style Chisel._ Bundle definition:

class ChiselBundle extends Bundle {
  val a = UInt(width = 32)
  val b = UInt(width = 32).flip
}

used in this chisel3._ Module:

class Chisel3ModuleChiselBundleA extends Chisel3DriverModule(new ChiselBundle)

[sdtwigg]

If Chisel._ assumed everything is an output, how is the chisel bundle with Chisel._ different from this as a chisel3._?

class ChiselBundle extends Bundle {
  val a = Output(UInt(width = 32))
  val a = Flipped(Output(UInt(width = 32)))
}

[mwachs5]

"Nodes can have these directions: Input, Output, Bidirectional (for Aggregates, below), or Unspecified (default)."

What about Analog (InOut) type?

[ducky64]

@sdtwigg I'm not quite sure I understand. The Chisel._ ChiselBundle in my example should be equivalent to your ChiselBundle. However, the newly proposed semantics requires absolute direction somewhere, so nothing but a chain of flips (or even unspecified) would be an error in chisel3._.

@mwachs5 Analog is counted as Bidirectional.

[sdtwigg]

The reason why I ask is because then it seems you can just assume any fields in a Chisel.Bundle default to Output.

Analog counting as Bidirectional somewhat contradicts the parenthetical statement in "Bidirectional (for Aggregates, below)" implying that Bidirectional only shows up in Aggregates.

[ducky64]

Yes, though the difficulty is in the implementation, since Chisel.Bundle is equivalent to chisel3.Bundle, so there would need to be some way to differentiate them while maintaining interoperability (threading through compile options? having Chisel.Bundle be a sub-class of chisel3.Bundle?). It would also require compatibility code support in chisel3, which would be ugly, but there's already precedent in the auto IO wrap.

Analog was one of the things that I didn't really consider in the proposal and was caught by test cases. I've updated the spec above.

[colinschmidt]

I think VecWire is confusing because it is supposed to take a hardware input but both Vec and Wire take type inputs, I don't really have a better name VecHW?

Is the bundle assuming output problem intended to be the new solution to #495 ? Again I don't have a solution just a confirmation that it certainly an issue but has existed in some form for a while.

[colinschmidt]

I think the error out approach makes it very difficult to incrementally upgrade from Chisel._ to chisel3._

Is @jackkoenig's solution from the other issue as messy as your second solution?

Giving up doesn't seem like the right solution... :/

[ducky64]

@colinschmidt The Bundle Output problem is a new issue created by stricter checks. I guess #495 / #595 provides part of the implementation for solution #2, I'll try that. However, that still requires compatibility code in core, which is undesirable but not a dealbreaker.

Vec/VecWire: I see what you mean, and in that light, they're really not good names. VecHW seems very different than anything else we have so also seems undesirable.
There may be two similar cases on which to draw inspiration: Reg (which takes a type) and RegInit (which takes an init and optionally a type), and similarly Wire and Wire(..., init=...). It might not be a bad idea to rename the initialized wire to WireInit, so maybe VecInit?

[colinschmidt]

I think VecInit makes sense to me. You end up with a Vec that has the same initial value as your hardware input.

[aswaterman]

If we're renaming them anyway, we should rethink the names... init currently means something very different for Wire and Reg. Wire(init = ...) is actually much more similar to the behavior of Reg(next = ...) than it is to Reg(init = ...), in that it provides the default value on that clock cycle if no other value is given. In other words, right now we have that

• RegNext(x) is equivalent to val y = Reg(x.toType); y := x
• but WireInit(x) is equivalent to val y = Wire(x.toType); y := x

and init indicates a dependence on reset for Reg but not Wire.

[colinschmidt]

Does that mean WireNext and VecNext would be the new names?

[aswaterman]

I don't know, that's not great either, but it is certainly more consistent.

[jackkoenig]

I strongly agree with @colinschmidt that we should prioritize allowing standard Chisel._ Bundles with implied output direction. Perhaps using compile options we can autowrap the elements upon construction? As we discussed in person, it's not clear how these implicits work when you construct a Chisel._ Bundle inside of chisel3._ code, need to investigate.

[jackkoenig]

Perhaps the problem is that init and next really only have meaning for a Reg. What if Wires were different altogether with WireDefault or something?

[mwachs5]

Hmmm, but for RegNext I guess as a user I never even considered overriding the returned value. I assumed the "Next" meant that I was sure I always wanted it to take the Next value. Otherwise what is the point of RegNext. WireDefault and WireInit both make sense to me, but maybe that's just cuz I'm used to passing in init =

[aswaterman]

WireDefault makes more sense to me

[jackkoenig]

I made a little example of implicits on a common supertype that takes an implicit and is then extended by subtypes that materialize this implicit and it seems to work just fine. By making Bundle (and record) accept an implicit CompileOptions, we then know if the subclass was defined in chisel3._ or Chisel._. Upon performing a chisel3 bulk connect, we can check if the Bundle is a compatibility bundle. If so, we can give unspecified elements the default Output direction then. This would be a relatively easy improvement on #595 anyway.

[ducky64]

(probable bikeshedding!) WireDefault makes sense, but sounds a little verbose. Character-wise, Default is longer than Wire, Vec, IO, Reg, Mem and gives the impression as more of a fallback value (which is how it's used in C/C++ switch statements).
That being said, I can't think of anything better right now, though. I think init makes sense (but imperfectly), since you are giving the Wire / Vec an initial value. The binding stuff / hardware is essentially implementation details.

[ccelio]

I find both WireDefault and Wire(init=...) completely unoffensive and fairly intuitive.

With that said, I actually like Wire(init=...)'s symmetry with Reg(init=...). I don't know what people's preferred style is, but grepping through my copy of rocket, I see Reg(next=, Reg(init=, and RegEnable.

Also, I agree with @mwachs5 regarding RegNext: I have no idea what the precedent rules are and always assumed whatever was passed in dominated. Even if that's not the case, it seems poor form to use Next with other overloading assignments.

[ducky64]

Ok, so here's what we currently have:

Wire(t) only takes a type, the Wire(init=...) will be deprecated and replaced with WireInit(init) or whatever we decide on
Reg(t) only takes a type, the Reg([next=...], [init=...]) has already been deprecated and replaced with RegNext(next), RegNext(next, init) and RegInit(init), RegInit(t, init). This mostly avoids precedence rules that plagued the old Reg.
Vec(t) only takes a type, the initialized Vec will be replaced with VecInit(elts*) or whatever we decide on.

There's also a few things in chisel3.utils:

RegEnable(next, enable)
RegEnable(next, init, enable)
ShiftRegister(in, n, [enable])
ShiftRegister(in, n, resetData, [enable])

In the larger picture, I'm not sure there's a clear consistent philosophy to how we order arguments, though we can certainly try to change the APIs and fix existing use cases. Suggestions for what things should look like are also welcome.

[chick]

This looks reasonable to me, it seems logical and fairly consistent.
Perhaps a guide for argument conventions would be good, but I'd guess one would experience deeply diminished returns on putting more than an hour into that effort, seems like a definitive effort would be intractable.
Minor note: It's no big deal but RegEnable and ShiftRegister being util is a little confusing from an external perspective, since they seem to fit right in with the other register functions. The reason they are in a different package is because of internal implementation details. When possible that detail should not show up in external APIs