Determine how to handle non-Verilog legal Extmodule DefName, i.e., Literal Identifiers

[seldridge]

A circuit that includes a Verilog-illegal defname in an extmodule should be handled or rejected. This depends on the interpretation of whether the defname is the "Verilog name" or if it is an identifier used to uniquely group multiple external modules together. Practically, it's the former.

Consider:

circuit Foo:
  extmodule Bar:
    defname = 42_Bar
  module Foo:
    inst bar of Bar

For now, let's just reject this and require Verilog-legal names.

How should this work for literal identifiers? Is this any different?

[JL102]

Related discussion: ucb-bar/chipyard#1542

After checking the behavior of the original FIRRTL tool, by using the Chisel bootcamp notebook, it appears that it adds an underscore to the end of any instance names which are a reserved word in Verilog. Test with this code:

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

class Passthrough2 extends Module {
    val io = IO(new Bundle {
        val in = Input(UInt(4.W))
        val out = Output(UInt(4.W))
    })
    val module = Module(new Passthrough())
    module.io.in := io.in
    io.out := module.io.out
}

Change the name module to any Verilog reserved word and it appears that the original FIRRTL tool will just add an underscore to the end, for example:

module Passthrough(
  input  [3:0] io_in,
  output [3:0] io_out
);
  assign io_out = io_in; // @[cmd24.sc 6:12]
endmodule
module Passthrough2(
  input        clock,
  input        reset,
  input  [3:0] io_in,
  output [3:0] io_out
);
  wire [3:0] module__io_in; // @[cmd24.sc 14:24]
  wire [3:0] module__io_out; // @[cmd24.sc 14:24]
  Passthrough module_ ( // @[cmd24.sc 14:24]
    .io_in(module__io_in),
    .io_out(module__io_out)
  );
  assign io_out = module__io_out; // @[cmd24.sc 16:12]
  assign module__io_in = io_in; // @[cmd24.sc 15:18]
endmodule

I tested with a handful of random Verilog reserved words, namely and, always, and endtask (took from this page: https://redirect.cs.umbc.edu/portal/help/VHDL/verilog/reserved.html)

If it's a non reserved word, there'll be no underscore added to the end. I believe that this exact behavior should be added to the spec, so that firtool does not crash when any previously legal instance names are used.

EDIT: Looks like firtool adds _0 to the end of instance names with some reserved words, like and. This behavior should probably be legal, too. Maybe the spec should have wording something along these lines:

If the name of an instance is a reserved word in Verilog, such as module, always, or fork, then a predictable set of characters should be appended to the end of the name, for example transforming module to module_ or module_0.

Maybe it can go into the Instances section of the spec?

[seldridge]

Thanks for the detailed post. The issue you bring up is slightly different from the issue I was worried about here. I'll comment on the issue I was worried about and then address your points.

Are you seeing any crashes in firtool or using an old version of firtool? This should be both properly parsing trickier FIRRTL syntax like this and avoiding Verilog keyword collisions via implementation-defined name mangling.

My Issue

What I'm worried about is if a user says, "This external module has a Verilog-exact defname that isn't a legal Verilog name." I was concerned about this for literal identifiers (identifiers which start with a number and are not legal Verilog identifiers), but the problem is the same for Verilog keyword collisions. Consider:

circuit Foo:
  extmodule Bar:
    output a: UInt<1>
    defname = module

  module Foo:
    output a: UInt<1>
    inst bar of Bar
    a <= bar.a

The defname is saying, "The verilog name of this external module is module." This is impossible. This seems like it has to be an error. The compiler could mangle the name, but unless that mangling is exactly specified in the spec then the compiler may cause Verilog link problems later. If the user is relying on the mangling behavior here, then that's also weird because they should've just used the exact post-mangled name (which they must have known because they had a Verilog module with that name!).

For the above code, the SFC (maintenance mode Scala-based FIRRTL Compiler) will produce illegal Verilog:

module Foo(
  output  a
);
  wire  bar_a;
  module bar (
    .a(bar_a)
  );
  assign a = bar_a;
endmodule

CIRCT/MFC (MLIR-based FIRRTL Compiler) will produce equivalent illegal Verilog and print an error during Verilog emission:

# firtool firrtl-115.fir 
<unknown>:0: error: name "module" is not allowed in Verilog output
<unknown>:0: note: see current operation: 
"hw.module.extern"() ({
}) {argLocs = [], argNames = [], arg_attrs = [], comment = "", function_type = () -> i1, parameters = [], res_attrs = [{}], resultLocs = [loc(unknown)], resultNames = ["a"], sym_name = "Bar", sym_visibility = "private", verilogName = "module"} : () -> ()
// Generated by CIRCT unknown git version
// external module module

module Foo(
  output a
);

  module bar (
    .a (a)
  );
endmodule

Your Issue

The issue you are bringing up is more about specifying the name mangling behavior of a FIRRTL compiler. The new FIRRTL ABI document talks about this a bit for certain things. The current language around modules is:

Any module considered a "public" module shall be implemented in Verilog in a
consistent way. Any public module shall exist as a Verilog module of the same
name.

This pedantically means that any public module, e.g,. the main module, needs to have a legal Verilog name in order for it to be compiled.

One place that this is not fully specified is for ports of public modules which are illegal Verilog names. @darthscsi: thoughts on what to do here?

The issue that you bring up about an internal instance name is more in the realm of, "The compiler can do whatever it wants so long as the output is legal Verilog". I.e., I think the name mangling algorithm of internal things should be implementation defined.

[JL102]

I see, thanks for clarifying the difference between the two issues.

Are you seeing any crashes in firtool or using an old version of firtool? This should be both properly parsing trickier FIRRTL syntax like this and avoiding Verilog keyword collisions via implementation-defined name mangling.

Actually, I just checked the most recent release of Firtool and it looks like the issue I was seeing is present in firtool version 1.30, but was fixed by the most recent version, 1.45. Version 1.30 is what Chipyard is using, since it's the most recent version published to Anaconda. In Firtool v1.45, it no longer gives the error 'module' must be first token on its line with the sample code I tested. I'll ask them why it hasn't been updated on the Anaconda repo.

[seldridge]

I'll ask them why it hasn't been updated on the Anaconda repo.

👍 Sounds good. We aren't coordinating any releases through Anaconda (and I wasn't even aware that CIRCT was getting released there 😅 ). @jackkoenig has been working on improving CIRCT (firtool) publishing and we should be able to have a version that you can get as a Maven package. I.e., there would be no need for a visible external dependency and a version of firtool compatible with Chisel will be automatically downloaded for you.

[JL102]

I'll ask them why it hasn't been updated on the Anaconda repo.

👍 Sounds good. We aren't coordinating any releases through Anaconda (and I wasn't even aware that CIRCT was getting released there 😅 ). @jackkoenig has been working on improving CIRCT (firtool) publishing and we should be able to have a version that you can get as a Maven package. I.e., there would be no need for a visible external dependency and a version of firtool compatible with Chisel will be automatically downloaded for you.

Nice, that sounds useful. After I wrote the reply, I found out that the Anaconda package is from some folks from UC Berkeley, which seems to grab the Github release and publish it to the Anaconda repo: https://github.com/ucb-bar/firtool-feedstock
Unfortunately it was published a single time with release 1.30.0, and then never again. It seems to me that the reason they put it on Anaconda was to make it easier to integrate it with Chipyard (since it's already using Anaconda for most of its dependencies).