Add Public trait to create public FIRRTL modules

[seldridge]

Add a new trait, Public, which can be used to emit FIRRTL modules which have the public keyword. This is a weak form of "public modules" that we want to support for FIRRTL 4.0.0. This does not allow for multiple, disjoint instance graphs to be contained in a circuit, nor does it remove the "main module" from the circuit. However, it provides a mechanism for plucking modules out of a circuit later and working with them.

This is self-typed on RawModule to prevent mix-in to an external module.

For the included test Chisel:

class Baz extends RawModule

class Bar extends RawModule with Public {
  val baz = Module(new Baz)
}

class Foo extends RawModule {
  val bar = Module(new Bar)
}

This produces the following FIRRTL. Note the public keyword:

FIRRTL version 4.0.0
circuit Foo :
  module Baz :

    skip

  public module Bar :

    inst baz of Baz

  public module Foo :

    inst bar of Bar

And Verilog. Note that Bar is not eliminated because it is public while Baz is:

// Generated by CIRCT firtool-1.65.0
module Bar();
endmodule

module Foo();
  Bar bar ();
endmodule

Release Notes

Add Public keyword to create public FIRRTL modules.

[jackkoenig]

I'm not sure that this necessarily should be a static property of the class. It seems plausible that you may want a given module to be public in some configurations but not necessarily in others. The biggest example of this is whatever your top module is which is certainly public for the build where it is the top, but may not be public in other builds.

We could do something like what OpaqueType does where mixing it in defaults a protected def to true but the user can still override it to calculate whether they want it to be true or false depending on Scala-time parameters.

[mwachs5]

but couldn't you get the same behavior with a pattern like:

Module(new MyNormalModule with Public)

Isn't chisel emission going to have to de facto do this if you want to emitSystemVerilog(new Queue)?

[jackkoenig]

but couldn't you get the same behavior with a pattern like:

Module(new MyNormalModule with Public)

You could, but if MyNormalModule takes several parameters, this is getting a bit boilerplate heavy for dubious utility, eg.

val module = if (public) {
  Module(new MyNormalModule(arg1, arg2, "foobar", arg3) with Public)
} else {
  Module(new MyNormalModule(arg1, arg2, "foobar", arg3))
}

My suggestion is that if you mix in Public, it defaults to being public, but you can override a def to control the behavior:

class MyNormalModule(args..., public: Boolean) extends Module with Public {
  // Only in cases where I may not want to be public do I need to do this:
  override def isPublicModule = public
}

We originally did the anonymous thing for [OpaqueType](

chisel/core/src/main/scala/chisel3/experimental/OpaqueType.scala

Line 44 in cba3158

def opaqueType: Boolean = true

but it was very painful in practice so we changed to exactly what I'm suggesting here which has worked well.

Isn't chisel emission going to have to de facto do this if you want to emitSystemVerilog(new Queue)?

Pretty much, which is why thinking about it purely as a static property of a given class doesn't make sense to me since it is very context-dependent for how you are using the generator. I do think making it a trait is still good since much of the time it will be a static property of the class, but giving that little bit of control to the user is helpful.

[jackkoenig]

The words of this test seem contradictory with the behavior, Foo is not being marked public but shouldn't it be?

[seldridge]

Clarified below. In emission (at this point in time for the PR and later changed), Foo does not have the "public" keyword. However, it's the main module, so it has different semantics in that it is treated as if it were public. It's less confusing to not do it this way and was changed in a later fixup.

[jackkoenig]

I pushed a commit that does what I'm saying, which should be included in a test. Main concern is that the test seems to show us not marking the top module as public, but shouldn't we be?

[seldridge]

Main concern is that the test seems to show us not marking the top module as public, but shouldn't we be?

From the FIRRTL spec or CIRCT perspective with how FIRRTL 4.0 is shaping up, it doesn't matter. The test was more checking the exact emission style. I.e., that the code was not forcibly making the main module public. This is FIRRTL 4.0 not as the spec is written now, but in the way that we are planning to do it where modules can have a public attribute, but the circuit still has a main module.

If a circuit has a main module, then it is as if it is public from a compilation perspective---it has an implicit user and cannot be removed or have its ports removed. Whether or not it is marked public doesn't really matter as you can't have a "private" main module. We can then make a call in the spec on whether to make this publicness implicit, mandatorily explicit, or optional and without effect.

CIRCT doesn't actually care about this right now and it will accept circuits where the main module is public. Internally, the main module is always treated as having a public symbol.

I updated the PR to set the main module as public to show what this would look like.

[jackkoenig]

Looks great, thanks!

[seldridge]

Thanks for the review, Jack. I'm going to update the test to be more descriptive of what it is testing and then merge.