Deprecate Module.io and BlackBox.io

[jackkoenig]

Why?

I was experimenting with some stuff in Scala 2.13 and realized that -Xsource:2.11 is no longer supported in 2.13. In fact, it was never recommended as an option to Scalac for anything but debugging, let alone our recommended way of supporting Scala 2.12. Thus I had a discussion on the Scala Contributors Gitter to figure out what to do.

For a long time, we thought the issue only had to do with structural typing. This was incorrect, there were actually 2 different problems:

1. Structural Typing
   • formerly unsupported in Scala 3, now supported, it always worked in Scala 2.12/13
2. Type Inference for overridden definitions defaults to overridden type (as opposed to overriding type)
   • Strict in Scala 3, weaker in Scala 2.12/13

The thing that made this confusing was that fixing either (1) or (2) is sufficient to no longer need -Xsource:2.11, but we generally only talked about solutions to (1). To be more precise, if we stopped using the pattern val io = IO(new Bundle { ... }), we would no longer need -Xsource:2.11 in Scala 2.12.

This solution could work as a ScalaFix rewrite rule:

class MyModule extends Module {
  val io = IO(new Bundle { ... })
}

Becomes

class MyModule extends Module {
  class MyModuleIntf extends Bundle { ... }
  val io = IO(new MyModuleIntf)
}

Unfortunately, due to (2) where Scala 3 is stricter, you would also need to annotate the type of io in Scala 3:

class MyModule extends Module {
  class MyModuleIntf extends Bundle { ... }
  val io: MyModuleIntf = IO(new MyModuleIntf)
}

While this is a solution, there is a massive downside that we have to encourage all of our users to rewrite all of their code (potentially ScalaFix assisted), and purge documentation of this bad style.

It turns out that removing io as a virtual method solves both (1) and (2). The exact same style of Modules with anonymous Bundles for io will work in Scala 2.12, 2.13, and 3.

Implications

What does this actually do?

This deprecation warning only shows up for when io is used as a field of Module or BlackBox itself. That is, only when relying on the common interface of those abstract classes will cause the warning. Some examples:

class Foo extends Module {
  // No warning
  val io = IO(new Bundle {
    val in = Input(Bool())
    val out = Output(Bool())
  })

  io.out := ~io.in // No warning
}

object Util {
  def connect(mod1: Foo, mod2: Foo): Unit = {
    println(s"mod1 has io: ${mod1.io}") // No warning
    mod1.io.in := mod2.io.out           // No warning
  }

  def connect2(mod1: Module, mod2: Module): Unit = {
    println(s"mod1 has io: ${mod1.io}") // Warning!
    mod1.io <> mod2.io                  // Warning!
  }
}

Thus, this warning is actually fairly rare because most people either rely on more type-safe APIs (some common trait defining a more precise type for IO), or use DataMirror.fullModulePorts since they want to be able to get the IO from MultiIOModules and RawModules as well.

Module

The virtual method allowed us to use the type system to enforce the single-source of ports via val io. We can still enforce that through DataMirror.fullModulePorts if we want. We could alternatively unify Module and MultiIOModule and relegate val io to a stylistic choice.

BlackBox

BlackBox has special behavior where the io prefix is dropped. While it's still an open-question for how to provide this behavior in a less hacky way, it needs to be maintained. The io virtual method is currently used to find the ports, but we could use the DataMirror.fullModulePorts "reflective" API to find them and ensure the io prefix is dropped.

Compatibility-mode

import Chisel._ does not require IO wrapping of io for Module and BlackBox. It uses the virtual method to apply the binding to them. 2 possible solutions:

1. Use reflection to find io
2. Use the compiler plugin to wrap them

Related issue:

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

Release Notes

Deprecate Module.io and BlackBox.io virtual methods

[ducky64]

Wow, I'm pleasantly surprised how simple the solution came out to be (assuming this doesn't break a giant chunk of code that we don't know about yet - though I agree with the arguments presented of why it shouldn't). I think this looks sane.

Other Comments

With regards to alternate solutions: I think it's worth thinking about useful properties of the current (anonymous Bundle) syntax in considering trade-offs. It:

• saves a line of class declaration
• hints that the Bundle isn't meant to be used elsewhere
• and, eliminates the need to come with a class name for a one-of class (and avoids boilerplate names like MyModuleIntf)

Separately from the 2.12+ compatibility issue is whether to unify MultiIOModule and Module - and if this PR goes through, is probably more of a style guide issue. I think the arguments for Module are mainly that it centralizes all the IO in one Bundle, so you can do bulk operations on them (but importantly, this doesn't include clock and reset, but they're usually implicitly connected the same source). However, the issue there is that extending the interface in subclasses becomes a lot harder, since you have to modify io instead of just adding a new val - which is what MultiIOModule makes easierr.

For BlackBox: doesn't ExtModule present a more MultiIOModule like interface, without the need for the .io single object and the naming hackiness? I don't know how much traction / usage it's seen, but it's a bit more elegant, or at least consistent?

[jackkoenig]

Wow, I'm pleasantly surprised how simple the solution came out to be

I cannot express how relieved I am at how simple it came out. This has been a big worry for a long time and it seems we get off basically scot-free.

assuming this doesn't break a giant chunk of code that we don't know about yet

Yeah I think it's important to look out for such issues on 3.4.0-RC1. Also if there is something we don't notice till after full release, it seems pretty easy to undo so I think the risk is low. I did try this with SiFive's codebase and only had like 10 warnings from it and they were all easily fixable cases. For example:
https://github.com/chipsalliance/rocket-chip/blob/2bdb03dbca3f77ad4c378cc1b95ab4961bc1448a/src/main/scala/prci/TestClockSource.scala#L79
The type of source is inferred to be Module with { io : ClockSourceIO }, easily fixable by defining a trait that provides that type for io.

I agree with your Other Comments. I think this is a good reason to just unify Module and MultiIOModule anyway.

For BlackBox: doesn't ExtModule present a more MultiIOModule like interface, without the need for the .io single object and the naming hackiness? I don't know how much traction / usage it's seen, but it's a bit more elegant, or at least consistent?

This one is a little tricker because people like the io stripping behavior of BlackBox. The problem with just flattening everything out in ExtModule is that then you can't just easily bulk connect to things. Imagine a complex AXI interface with dozens of signals that needs to be flattened out, but on the Chisel side you'd like to use a Bundle and be able to bulk connect to the ports of your ExtModule. This is my oft-proposed "prefixless Bundle" API. In any case, we can think more about this later.

[ducky64]

One suggestion for improved description

[ducky64]

Consider adding a bit more description here, something to the effect of "Module.io will be removed since it breaks in Scala 2.12+; you can still define io Bundles in your Modules, but cannot rely on a io field in every Module; for more details see (url here, possibly to this PR)"

And repeat below for Module.

[jackkoenig]

Good idea, I more-or-less copied your suggestion.