Lower bi-directional bundles to interfaces

[mikeurbach]

Support for lowering bundles to RTL structs was added in https://github.com/llvm/circt/pull/533. Structs cannot support bi-directional bundles, but SV interfaces can. Lowering bundles in this way was discussed a while back on Discourse: https://llvm.discourse.group/t/firrtl-aggregate-types-discussion/1806/17.

I propose to separate the lowering such that passive bundles always use structs, and use interfaces otherwise. It seems like the pieces for this are mostly in place already, and I'll start to take a look.

[mikeurbach]

I started looking into this, and have a rough idea of how I'd do it. For example, consider a simple test like this:

circuit Foo:
  module Foo:
    input source: {valid: UInt<1>, flip ready: UInt<1>, data: UInt<32>}
    output sink: {valid: UInt<1>, flip ready: UInt<1>, data: UInt<32>}
    sink <= source

The changes I'm proposing are twofold.

First, the lower-firrtl-to-rtl-module pass can create interfaces in the top-level module and rewrite the modules to use the interfaces as the ports' types. It would then insert casts back to the original FIRRTL types, leaving something like this:

  sv.interface @myInterface  {
    sv.interface.signal @valid : i1
    sv.interface.signal @ready : i1
    sv.interface.signal @data : i32
    sv.interface.modport @dataIn  ("input" @valid, "output" @ready, "input" @data)
    sv.interface.modport @dataOut  ("output" @valid, "input" @ready, "output" @data)
  }
  rtl.module @Foo(%source: !sv.modport<@myInterface::@dataIn>, %sink: !sv.modport<@myInterface::@dataOut>) {
    %0 = firrtl.svModportCast %source : (!sv.modport<@myInterface::@dataIn>) -> !firrtl.bundle<valid: uint<1>, ready: flip<uint<1>>, data: uint<32>>
    %1 = firrtl.svModportCast %sink : (!sv.modport<@myInterface::@dataOut>) -> !firrtl.bundle<valid: flip<uint<1>>, ready: uint<1>, data: flip<uint<32>>>
    firrtl.connect %1, %0 : !firrtl.bundle<valid: flip<uint<1>>, ready: uint<1>, data: flip<uint<32>>>, !firrtl.bundle<valid: uint<1>, ready: flip<uint<1>>, data: uint<32>>
    rtl.output
  }

Second, the lower-firrtl-to-rtl pass will need to resolve the casts and lower to RTL/SV dialects. The connect between two bundles in the above example could be lowered to the appropriate SV interface signal read/assign ops, like this:

  sv.interface @myInterface  {
    sv.interface.signal @valid : i1
    sv.interface.signal @ready : i1
    sv.interface.signal @data : i32
    sv.interface.modport @dataIn  ("input" @valid, "output" @ready, "input" @data)
    sv.interface.modport @dataOut  ("output" @valid, "input" @ready, "output" @data)
  }
  rtl.module @Foo(%source: !sv.modport<@myInterface::@dataIn>, %sink: !sv.modport<@myInterface::@dataOut>) {
    %0 = sv.interface.signal.read %source(@myInterface::@valid) : i1
    sv.interface.signal.assign %sink(@myInterface::@valid) = %0 : i1
    %1 = sv.interface.signal.read %sink(@myInterface::@ready) : i1
    sv.interface.signal.assign %source(@myInterface::@ready) = %1 : i1
    %2 = sv.interface.signal.read %source(@myInterface::@data) : i32
    sv.interface.signal.assign %sink(@myInterface::@data) = %2 : i32
    rtl.output
  }

Or, in System Verilog:

interface myInterface;
  logic valid;
  logic ready;
  logic [31:0] data;
  modport dataIn(input valid, output ready, input data);
  modport dataOut(output valid, input ready, output data);
endinterface

module Foo(
  myInterface source, sink);

  assign sink.valid = source.valid;
  assign source.ready = sink.ready;
  assign sink.data = source.data;
endmodule

Does this general approach sound reasonable? If so, I have the sketch of this in a branch that I can share.

I also have a question about the interfaces that get created.

To me, the passive type of a bundle should uniquely map to one interface, which should be shared. For example, in a design that bundles a 32-bit data signal with valid and ready bits in many places (Handshake), it would be nice to re-use one interface everywhere the bundle type gets used. Similarly, the type of a bundle (with flips) should uniquely map to one modport within the interface the bundle maps to.

If it works like this, a complex design coming from something like Handshake would boil down to a few interfaces with two complementary modports each, which seems desirable.

Does the idea of re-using interfaces and modports make sense? If so, I've prototyped a couple ways to do it: either by combining signal names, types, and directions into unique identifiers, or by simply adding an attribute to let the higher level dialect indicate what interface/modport identifier to use for each bundle. I suspect it would be good to do the former as a start, and support the latter as a way to have more control over the names in the output.

[lattner]

Something like this is cool, but please make sure it is optional guided under a flag of some sort (e.g. a pass flag). This is trending into the "verilog dialect" sort of area, given that some clients won't want interfaces because downstream tools won't support them (or just due to personal preference). This is great to explore though!

[lattner]

Also, yes, it seems vaugly reasonable to me, but i'm not an expert on interfaces :-)

[mikeurbach]

Other than lowering from Handshake, I don't actually have any use case at the moment that would use interfaces, so I will table this for now.