Module Hierarchy Refactor: Request for Comments

[ducky64]

Turns out the module hierarchy refactor is trickier than we originally thought, and there are several major decision points to be made. Feedback, thoughts, and example code (and especially example code) are welcome!

Allow multiple top-level IOs?

Should multiple top-level IOs be allowed? Currently, we only allow the io Bundle, but the infrastructure is partially there to allow IOs other than io.

Side note: clock and reset are implemented as top level IOs, but often aren't used as such (instead, it's common to use override_clock and override_reset, or soon withClock blocks).

Pros:

• Can define IO directly in traits (each trait can specify a different IO name), without through one level of indirection by constraining the type of io.
• Consistent with the clock and reset implementation, though practically users will use the coming-soon withClock blocks. With this method, clock and reset can be implemented with an ImplicitClock trait that is mixed in to an ImplicitModule, and users can define their own ImplicitX trait for things they want automatically hooked up.
• Strictly more functionality compared to the current single io scheme.

Cons:

• Requires reflection, see below
• No programmatic top-level IO. All top-level IO have to be defined as a val inside the top level (not in a for loop / if statement) of a module body. This is a restriction of the Scala type system, the IO obviously has to be a class method to be reference-able outside.
  • Option types might allow some programmatic top-level IO generation
  • Top-level IOs can still be of Record type.
• override val io would be disallowed and fail to compile (though this is probably a bad idea anyways). The pattern of constraining io type in superclasses (without giving it a concrete value, except at the leaf class) would still be fine.
• No bulk connect on all of a Module's top-level IO ports.
  • But such a bulk connect across very different IO ports might not make sense.
  • One way to mitigate this would be methods that make Bundles from top-level IO ports.

Style:

• Constraining to a single io Bundle forces all IOs to be defined at once place (good for readability), though style guides may limit how messy multiple top-level IOs can be

Can we unify naming of BlackBoxes and ImplicitModule

Currently, BlackBox drops the io_ prefix when generating names for IO ports, while Modules keep the io_ prefix.

In the single top-level IO case, one solution is to drop the io_ prefix from Modules. Some collisions are possible:

• clock and reset in the io Bundle would conflict with the implicit clock and reset signals. This could just error out (as having a clock and reset member in an IO Bundle is confusing).
• There is a significantly increased possibility of a naming collision with an internal Module wire. There is de-duplication logic in place for that, though it may get triggered more and can impede debuggabitlity.

Dropping the prefix from multiple top-level IOs is probably a really bad idea. In that case, an alternative would be for BlackBoxes to also use multiple top-level IOs to name pins without the io_ prefix.

• This does prevent programmatic generation of pins, though. I'm not sure how often this will be used. Vecs would still be allowed, and that may be a superior method.

Leaving it as-is (inconsistent) is a possibility, though undesirable.

Are we OK with IOs requiring reflection?

One implementation for top-level IOs requires reflection to properly name the top-level IOs. Module reflection may be significantly more difficult to do than Bundle reflection, where things are structured uniformly.

That being said, top-level IOs are usually defined similarly to how Bundles define members.

Thoughts?
@sdtwigg @shunshou @aswaterman

[aswaterman]

To me, the overriding concern is the stylistic one; I consider it a great feature that the Module's interface is specified in one place.

I realize it doesn't work for black boxes, but waveform debugging of Chisel-generated Verilog is usually simplified by module interfaces having the common prefix io_.

I would prefer not relying on reflection in even more places.

[azidar]

One thought is the current cake pattern + single val io forces the user to create an excessive amount of bundle traits, while multiple io declarations could resolve this.

Suppose I want to declare a Module trait that back-propagates an input ready signal to an output ready signal. Currently, I'd need to 1) create the Module trait to contain the hardware connection, and 2) create a new Bundle trait that ensures my io has the two ready ports I need.

If multiple io's are allowed, then I only need to make the Module trait, which can directly add both ready ports, as well as the connection. No new Bundle trait needed.

We see this all the time in rocket-chip, where we have a crazy number of bundle traits. I suspect this is because there is no easy way to append ports to modules, (like there is a way to append hardware to modules via traits).

[azidar]

I understand the concern of having all io declared in one place, but one could argue that even now, not all io is declared at the top of a module.

In rocketchip, we usually instantiate a custom bundle, which abstracts away all of the leaf io ports anyways. If I actually want to know what the ports are, I need to track down wherever that bundle is declared. This is distinctly not having all IO inside the module.

Software inheritance intentionally allows users to declare public methods/val's not with the concrete class. I suspect that the benefits of allowing more powerful Module traits outweighs the bad style of randomly declaring a port in the middle of a module. This is something we can certainly code review/style check for.

[mwachs5]

Right. The 3-part extension everywhere is kind of tiresome. It would also be really nice to be able to punch ports easily by just mixing in a "debugging ports" trait up the hierarchy.

I do second the request to keep things called "io_", but perhaps that now becomes a burden on the designer to follow a naming convention...

I assume that if this was enacted, every time I wanted to talk about a module's IO from the parent module, I have to know the right name for that part of the IO? As in (assuming my guess at your syntax is right):

class FooModule extends Module{
  val ioA = IO(new Bundle {val a = Output(1.B))}
  val ioB = IO(new Bundle {val b = Input(1.B))}
}

class BarModule extends Module {
  ...
  val foo = Module(new FooModule())
  val a = foo.ioA.a
  foo.ioB.b := false.B
}

[azidar]

Finally, allowing users to declare separate io's would allow them to actually comment their io's.

We could enforce an markdown-style comment for io's, which would allow a detailed explanation of the port's behavior.

[mwachs5]

"There is a significantly increased possibility of a naming collision with an internal Module wire. There is de-duplication logic in place for that, though it may get triggered more and can impede debuggabitlity.
"

What exactly is the problem here? Can it just be flagged as an error?

[azidar]

class BarModule extends Module {
  val io = IO( new Bundle {val r = Input(3.W))}
  val r = Reg(...)
}

If io_ is not appended, then r the port conflicts with r the register. The worry is that this name conflict would happen a lot, so we could either error, or rename the register to r_1.

[mwachs5]

Yeah that is confusing. It mirrors a problem in Verilog but it's less obvious because the r is so deep in some bundle you may not be aware of.

Not having the io_r name is suprising to say the least.

I guess I would have expected the following to be allowed:

class BarModule extends Module {
  val ioA = IO( new Bundle {val r = Output(1.B))}
  val fooB = IO( new Bundle {val r = Output(1.B))}
  val r = Reg(1.B)

  ioA.r := false.B
  fooB.r := true.B
  r := false.B

}

With this it would be pretty confusing to lose the names ioA and fooB in the generated code. Is this what you mean by needing reflection to properly name the ios in verilog?

Also, what is the difference between "IO" and just saying "Output" ? What happens if I put something that is not "Input" or "Output" in an IO, or just declare something as "Input" at the top level? (This is more understanding the proposed user view of this)

[chick]

I'm super confused. I thought the discussion was to make it so if you just wanted a port r you would do

val r = IO(Input(Bool()))

and that

val io = IO(new Bundle { val r = Input(Bool()) })

would give you io.r

[azidar]

There are multiple proposals going on here.

1. Multiple IOs. Allows you to declare val myIO = IO(...). No dropping of prefix myIO_.
2. Single IO, required to be declared as val io = IO(...). Dropping prefix io_.
3. Single IO, required to be declared as val io = IO(...). Keep prefix io_.

[aswaterman]

Please don't drop the io_ prefix for Module!! I understand the desire for RawModule, as a separate feature, but it is an anti-feature in the other cases, causing you to lose logical grouping of related signals. You wouldn't flatten a Wire bundle and destroy its name prefix. So it hardly makes sense to do that (by default) for the most important bundle in a Module.

…

On Tue, Feb 14, 2017 at 9:54 AM Adam Izraelevitz ***@***.***> wrote: There are multiple proposals going on here. 1. Multiple IOs. Allows you to declare val myIO = IO(...). No dropping of prefix myIO_. 2. Single IO, required to be declared as val io = IO(...). Dropping prefix io_. 3. Single IO, required to be declared as val io = IO(...). Keep prefix io_. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#507 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-7wqGRyVDgNcvnnMDTmSTUEDGzZUy8ks5rcepLgaJpZM4L_4tq> .

[ducky64]

Side note about use of reflection for IO ports: it's unlikely to fail since to have a useful IO, it needs to be a public object method (otherwise, you can't access it from outside), so it's pretty likely that it will work. There may be edge cases (like override val, but you really shouldn't be doing that anyways).

[sdtwigg]

I am extremely hesitant of the multiple IO idea and I am not OK with reflection actually becoming a critical feature (rather than nice-to-have). Users extending module already leads to leaky abstractions since they shouldn't actually access any fields except for the IO (and clock and reset). Now, with multiple IOs, there is no clean distinction. The user can access 'any' field that happens to be bound to an IO. This could be anything and isn't tracked in the type system so it can be a bit wild.

I disagree that the use of reflection will always work since you are now at the mercy of style guidelines to prevent the following abuses:

val stall = if(allowStall) IO(Input(Bool())) else false.asBool // confusing but tempting
val (a, b) = (IO(Input(Bool())), 1)  // macro approach will fail to assign name
val a = (IO(Input(Bool())), 1) // macro and runtime approaches will fail to assign name

I haven't followed the software engineering architecture of rocket-chip; however, the provided example was somewhat confusing. Rather than mixing in traits to the Module definition, why not instantiate submodules/call helper functions to do such work? The Record approach already allows for programmatically building up a Bundle. (In fact, you could still get your split IO for select modules by defining IO late in the module and building it up as a Record of parts defined earlier)

[mwachs5]

I understand your concern, but are you saying there is another way to get what we want (being able to add stuff to IOs when we extend a module.

Can we now instantiate submodules and then query what their IOs are and add them to our top-level?

What if I want to extend a module like:

class FancyModule extends OriginalModule {

 val fancyPeripheral = Module(new FancyPeripheral())
 fancyPeriperhal.io := io. ???
}

Are you saying that with Record I can now add whatever FancyPeripheral's IO is to the IO of OriginalModule's IO? How?

[sdtwigg]

I don't particularly suggest the approach for readability reasons but it requires the involved modules to have been designed for this purpose (hence why I was looking more at the trait-based approach).

The brief thought I had was:
The underlying base class involved (called say IncrementalModule) defines an empty ListMap[String, Data]. Traits (which must extend IncrementalModule) then do whatever logic and that ultimately connects to some wire. That wire is then added (with a describing pin name) to the ListMap. Finally, in the actual concrete module, it defines the io by cloning the elements of the ListMap as parts of a Record and connecting that IO to the originating wires (It might be possible to be clever with traits to define one that does this).

I'm not a huge fan of that notion and in the case you proposed would likely prefer having FancyModule instantiate and then connect up the OriginalModule. (Although, even in the inheritance case, you can have a habit of ancestor modules taking an io to use as a constructor argument so that children can enrich the IO with new fields.)

I should caveat that, for debugging purposes, I implemented #159 . However, the design is such that addPin is only called outside the other modules (almost always as some sort of library) rather than a thing that can be called by the current module. It is generally used to bubble up an internal signal to monitor in a testbench or force down some input override for testing.

[ducky64]

More on reflection:

• This will be no worse than Bundle reflection. The examples above (Chisel nodes in tuples) are not allowed in Bundles for the same reason. It's at least consistent with Bundle member detection / naming, for what it's worth.
• Because of the IO(...) wrapper, it is possible to detect if there are undetected ports hanging around in a module and give an informative error message. This is already better than the Bundle case, where it just fails silently (and presumably something else downstream catches fire).
• If a port is unnamed by reflection, it can be given a default name (like anonPort_1, etc). Whether this is a good idea depends on what our naming guarantees for modules are (which also depends on what people use them for) and whether there are subtle errors that can be prevented with strict checks,
• This provides a statically typed solution, where Records provide a dynamic solution that is inferior (in particular, checks fail at runtime and IDEs can't provide auto-completion support).

In the end, it's a choice of trade-offs between several imperfect solutions. The current way (single io port) isn't perfect (though we might be used to working around its limitations), and implementing multiple ports gives additional capabilities, though also with some additional cost.

[azidar]

Perhaps we need some experimentation before we can really understand the implications of multiple IOs. Is it possible to do solution 3 (one IO, keep io_ prefix), but allow multiple IOs if you import experimental?

Then, if it turns out people are hanging themselves with too much rope, we can deprecate the feature?

[ducky64]

I think it's possible, but requires a bit more engineering effort - the implementation will probably be much like compile options. Effort aside, I think it's a good intermediate solution until we fully understand the trade-offs we're making.

That being said, the BlackBox IO behavior will still be inconsistent, and that will be an API breaking change (unless we create a new BlackBox class or something).

[azidar]

I suppose we could do:

1. make the multiple IO's on BlackBox's not experimental
2. introduce a new BlackBox if we decide we like multiple IO's that does not drop the prefix
3. make a backwards incompatible change if we decide we like multiple IO's

I vote for 2, then 1, then 3.

[ducky64]

What would you call the new BlackBox? Naming is hard...

[colinschmidt]

I think BlackBoxes is one place its most unlikely to want to have multiple IO's.
I personally like the argument Andrew made for keeping io_.
I'm not up on my scala reflection limitations enough so I won't comment on that.
I think that Records enable some of the "add additional IOs" functionality if you are willing to restructure your code, so I think some experimentation is necessary.

[chick]

@ducky64 firrtl calls them external modules

[aswaterman]

I think it would be interesting to first push the limits on what we can do with Record.

[ducky64]

Record has its place where things must be programaticly generated, but you lose static typing (where you can catch errors can compile time and IDEs have code assist) so it's much less desirable if you want to do can be expressed staticly.

[azidar]

I think it would be interesting to first push the limits on what we can do with Record.

I don't see why we can't push both, and we can mitigate incorrect usages with forcing the import of experimental.

re @ducky @chick, yes I would call the new blackbox ExternalModule.

[azidar]

re @colinschmidt

I think BlackBoxes is one place its most unlikely to want to have multiple IO's.

I disagree, I think it is totally unnatural to declare your black box ports in the val io = ... format.

I personally like the argument Andrew made for keeping io_.

I think Megan's point that this can be something enforced by the designer is a good counterpoint to this. Another option is that we can add an optional Firrtl pass that appends the "kind" to every name in the design (like io_, reg_, wire_ etc.), for easier waveform viewing.

I'm not up on my scala reflection limitations enough so I won't comment on that.

Agreed, I don't know enough to comment, but it seems like its no worse than Bundle.

I think that Records enable some of the "add additional IOs" functionality if you are willing to restructure your code, so I think some experimentation is necessary.

Agreed, experimentation is good.

[sdtwigg]

First, to reiterate with detail, I retain concern about more critical reliance on reflection since I do not think reflection should be considered "unlikely to fail". See the situation leading up to #456 . (Also, that PR remains unmerged so it isn't like we can rely on these reflection issues being fixed promptly.) Also, to reiterate a different point: people do complain (consistently, even while still using Chisel) about all the badly named T and GEN wires. It is very convenient to be able to claim that they are merely internal signals whose name stability shouldn't be relied on too extensively (except perhaps for debugging) but that you can still rely on module IO names to be sane and user-driven.

Second, as an analogy, people rarely seem to complain that function arguments and return details have to specified in a singular place. Similarly, users can rely on the module IO at least being in one place (and in one file). A Module can inherit from a trait to do multiple things (which, at this time, does not include adjusting the IO) whereas an IO Bundle will clearly do so to add fields (constraining the user search).

Finally this point continues to be oddly asserted, Record does not necessarily lose static typing information in the same way Bundle does not lose static typing information. If all you know it is a Record, sure you don't know the types of the subtype; however, specific Record implementations can retain type information.

e.g.

class MyRecord extends Record {
  val a = UInt(16)
  val b = SInt(16)
  def elements = ListMap("a" -> a, "b" -> b)
  // insert cloneType def here
}

Retaining type information across the IO situation is trickier but doable. (The Record would likely have a 'polymorphic map' that contains pointers from the driving data wire in the extending trait and io wire designated to connect to it. This is access with def getIO[T <: Data](desired: T): T. The truly typesafe version technically requires this to return an Option[T]; however, T is likely more usable in actual use cases. You would access these hidden IOs with io.getIO(desiredPartialIOInTrait).) These extractions could even be done as a def inside the trait itself (as long as it is only called by the parent module, namely after the concrete subclass is fully constructed). The example is cursory and could use some refinement/exploration.

[azidar]

Superceded by #512.