Vec of Module IO strips directions and can connect to Bundle with different directions

[jackkoenig]

Consider the following:

class PassthroughModule extends Module {
  val io = IO(new Bundle{
    val in = Input(UInt(32.W))
    val out = Output(UInt(32.W))
  })
  io.out := io.in
}

class FakePassthroughModule extends Module {
  val io = IO(new Bundle{
    val in = Input(UInt(32.W))
    val out = Input(UInt(32.W))
  })
}

class TestCase extends BasicTester {
  val vm = Vec.fill(1)(Module(new PassthroughModule).io)
  val m = Module(new FakePassthroughModule)

  vm(0) <> m.io
}

Despite PassthroughModule and FakePassthroughModule having different directions, the Vec strips directions so this connection succeeds. Frighteningly, it even goes through Firrtl and emits valid Verilog.

[sdtwigg]

The Vec.apply(Seq) methods as currently implemented are not good since they attempt to simulate behavior from Chisel2 that FIRRTL simply doesn't support. FIRRTL requires all Vec be 'fresh' whereas Chisel2 Vec.apply could stitch up a Vec from pre-existing hardware components. Since these Chisel3 Vec.apply(Seq) methods are actually returning a wire, they are somewhat beholden to those semantics. (Wires, being internal nodes, don't really have a strong notion of directionality and thus are reassignable, even if driven from a submodule output.) The issue is somewhat compounded by all the old Chisel2 code lying around (or Chisel3 code that was 'ported' from Chisel2) that uses this behavior.

Perhaps it would just be better in the long run to fulfill the user needs with an appropriate set standard library methods. If the user doesn't need dynamic indexing, they should be using Vector.fill for these purposes. Otherwise, you could have a wrapper class with .read and .write commands that automatically issues the muxing and/or address-marked when statements as needed to simulate dynamic indexing. (Vec exploding will do this in the backend anyway).

The underlying problem (that has been existent for all of Chisel3) is that it presents itself like the Chisel2 api when it is currently doomed to behave differently.

[shunshou]

^ If the user doesn't need dynamic indexing (which is like... 99% of all of my old use cases for Vecs), Records have been amazing! I would just cut down on my support for Vec's i.e. only for things like ROMs, and tell users to use Seq or something "safe" instead... (which is what Stephen suggested to me when I was running into painful Vec problems several months ago...).

[jackkoenig]

There are some serious advantages to preserving Vecs through Firrtl (preserving them to the emitted Verilog for example). Obviously this is less relevant for Modules because Firrtl doesn't truly support Vecs of Modules, rather faking it with a Vec of a Wire connected to the modules' ports. (Also Verilog obviously does not support Vecs of instances, but you could imagine other backends might)

rocket-chip has an implicit conversion from Seq to AugmentedSeq which emits dynamic indexing directly rather than using it in Firrtl. We could provide this in chisel3 and make it illegal to create Vecs of bidirectional wires, instead pointing the user to use that.

All this being said, I feel like there some advantages to preserving Vecs of Modules. As previously mentioned, it might be useful to have this information in non-Verilog backends. It is also kind of inconsistent to have dynamic indexing handled by Vecs for everything but Modules where Chisel expands it. While it is true that Firrtl has a pretty weak notion of directionality for wires, I believe it does have a sufficient notion to do the right thing for bidirectional Vecs like this. @sdtwigg are there cases in Firrtl that straight up won't work and that we can't work around in Chisel?

[sdtwigg]

I strongly advise moving towards the AugmentedSeq at least for the short-to-medium term. It looks like the current version only supports dynamically-indexed reads though, so would need to consider how to handle dynamically indexed writes. Perhaps add a .write(UInt, T) field, although this won't work for bi-directional connectivity. (Chisel2 actually had an interesting mechanism where a single Chisel object could stipulate different emission behaviors when used as an rvalue versus lvalue. Unless Chisel3/firrtl was re-architected to support similar, you likely won't be able to get aseq(_: UInt) := _ <: Data to work as you'd expect....)

My critique is that Chisel3 has never acutally preserved Vecs of Modules. It had a Wire(Vec) that just so happened to be initially connected to module ios. However, since Chisel allows reassignment and doesn't error on unassigned wires, you can easily misuse this wire where the parity gets flipped and you are just reassigning submodule outputs while reading (but never driving) the inputs.

The problem with internal node directionality in firrtl is that it in practice it only seems 'half-correct'. It works great for all the examples presented to justify behavior but falls apart in the hands of the users. The underlying problem is that initially it was billed as a direct replacement for the bidirectional connect but had completely different semantics (notably, non-commutative).

As an example, it looks reasonable for a single DecoupledIO port since that has a clear single direction for data to flow. However, it starts looking really odd for two bundled but opposite DecoupledIO port (think request/response) and trying to reason about which of the client or server are the rvalue and lvalue.

Then, looking deeper, it was never totally clear what all the new semantics bought you over just using the old bi-directional connect and finer-grained modules, especially if you can tell the backend to inline some of those modules. At times, I have been tempted to having a bi-connect resolution phase in Firrtl so you could use it on internal nodes, e.g. int1 <> int2; in <> int1; out <> int2 would resolve. However, this has its own complications with registers (do they connect to the source or sink part, can't look at rvalue versus lvalue since want to maintain commutativity) and when statements.

In the end, just using modules, which have a clear directionality, ends up being simplest. For example, can have a Retimer modules that takes a bi-directional bundle as an argument (say assumed to be the enq side), has an io of enq and deq = flipped, then uses chisel reflection to put registers between enq and deq with the right connectivity. This hits the use case originally proposed for firrtl internal node connectivity without having to muddle with <> semantics.

[jackkoenig]

The issue now is fundamentally wanting to treat wires and ports as isomorphic in certain contexts. I think this should probably be handled with a new bulk connect operator that is sensitive to lhs and rhs (which is how <> works in Chisel and all connections work in Firrtl).

[azidar]

New link to AugmentedSeq.

New operator proposal (@jackkoenig): :<>, which is a directioned bulk connect. Benefit is it can be backed out later as it is a pure API addition (not change to existing API).

Alternative: make <> directioned only if connecting two wires. Could be confusing, however.