[FIRRTL] Set output dirs on annotated blackboxes

[seldridge]

Change the AssignOutputDirs pass to add hw::output_file attributes on
external modules in the same way that these attributes are added to normal
modules. While this has no effect on an external module which has no
implementation, this fixes a bug where the external module does have an
implementation that the compiler will later resolve.

This is specifically done to make output directories compose correctly
with Chisel inline blackboxes where the body of the external module is in
an annotation and is supposed to be written to a file.

Reorder the FIRRTL pass pipeline so that Chisel blackboxes (represented by
a firrtl.extmodule and one of two blackbox annotations) will be assigned
an output directory based on their instantiation location.

Fixes #7538.

[dtzSiFive]

I noticed the generated file list for black boxes contains these:

// ----- 8< ----- FILE "firrtl_black_box_resource_files.f" ----- 8< -----

A/Bar.sv

This seems functionally correct, and I don't know what these are used for, but this does suggest challenges re:deleting A/ and expecting the resulting design to be usable without modification.

These annotations and similar cannot go on parameterized modules, right (re:output dir attribute being different on different extmodule's that are really parameterizations of same (via defname))?

[seldridge]

@dtzSiFive wrote:

This seems functionally correct, and I don't know what these are used for, but this does suggest challenges re:deleting A/ and expecting the resulting design to be usable without modification.

A GitHub search acts like we don't have users of this internally. Externally, ChiselSim will use this file to know what extra files to copy to an input source area and will include these in compilation: https://github.com/chipsalliance/chisel/blob/5c129120ba06229e6c5d28c8120b35166770fa09/src/main/scala/chisel3/simulator/package.scala#L164

Practically, I think if the blackboxes move, so long as the blackbox filelist is updated to reflect these paths relative to the same directory as before, things will continue to work.

That said, the whole filelist approach is likely bad as it is, pedantically, undefined as to what should be included and doesn't work well with the FIRRTL ABI. It is likely saner to tell backend tooling to not use either of these filelists and instead rely on a glob include from the output directory. I.e., most tools support running them like: -I build/ --top-module Foo. (This will read all files and then elaborate from the specified top module.)

@dtzSiFive wrote:

These annotations and similar cannot go on parameterized modules, right (re:output dir attribute being different on different extmodule's that are really parameterizations of same (via defname))?

They can, yes. ExtModules cannot capture parameterization, only the arguments used to parameterize. Therefore, if you want to collapse multiple ExtModules down to a single parametric Verilog blackbox, you need to put these annotations on all of the ExtModules.

[dtzSiFive]

They can, yes. ExtModules cannot capture parameterization, only the arguments used to parameterize. Therefore, if you want to collapse multiple ExtModules down to a single parametric Verilog blackbox, you need to put these annotations on all of the ExtModules.

Okay, thanks. Probably you can see where I was going with that -- I was concerned about different parameterizations getting assigned different directories (and what happens if that's true) which seems like a problem as the following demonstrates:

FIRRTL version 4.0.0

circuit Foo: %[[
  {
    "class": "firrtl.transforms.BlackBoxInlineAnno",
    "target": "~Foo|Bar",
    "name": "Bar.sv",
    "text": "Bar definition that takes parameters"
  },
  {
    "class": "firrtl.transforms.BlackBoxInlineAnno",
    "target": "~Foo|Baz",
    "name": "Bar.sv",
    "text": "Bar definition that takes parameters"
  }
]]
  layer A, bind, "A":
  layer B, bind, "B":

  extmodule Baz:
    input a: UInt<1>
    defname = Bar
    parameter x = 1

  extmodule Bar:
    input a: UInt<1>
    defname = Bar
    parameter x = 0


  public module Foo:
    input a: UInt<1>

    layerblock B:
      inst baz of Baz
      connect baz.a, a

    layerblock A:
      inst bar of Bar
      connect bar.a, a

Which with this PR yields:

// Generated by CIRCT firtool-1.82.0-18-g1ceee216b
// external module Bar

// external module Bar

module Foo(
  input a
);

endmodule


// ----- 8< ----- FILE "B/layers_Foo_B.sv" ----- 8< -----

// Generated by CIRCT firtool-1.82.0-18-g1ceee216b
`ifndef layers_Foo_B
`define layers_Foo_B
bind Foo Foo_B b (
  .a (a)
);
`endif // layers_Foo_B

// ----- 8< ----- FILE "A/layers_Foo_A.sv" ----- 8< -----

// Generated by CIRCT firtool-1.82.0-18-g1ceee216b
`ifndef layers_Foo_A
`define layers_Foo_A
bind Foo Foo_A a_0 (
  .a (a)
);
`endif // layers_Foo_A

// ----- 8< ----- FILE "B/Foo_B.sv" ----- 8< -----

// Generated by CIRCT firtool-1.82.0-18-g1ceee216b
module Foo_B(
  input a
);

  Bar #(
    .x(1)
  ) baz (
    .a (a)
  );
endmodule


// ----- 8< ----- FILE "A/Foo_A.sv" ----- 8< -----

// Generated by CIRCT firtool-1.82.0-18-g1ceee216b
module Foo_A(
  input a
);

  Bar #(
    .x(0)
  ) bar (
    .a (a)
  );
endmodule


// ----- 8< ----- FILE "B/Bar.sv" ----- 8< -----

// Generated by CIRCT firtool-1.82.0-18-g1ceee216b
Bar definition that takes parameters
// ----- 8< ----- FILE "firrtl_black_box_resource_files.f" ----- 8< -----

B/Bar.sv

[seldridge]

Nice test case. Yes, this doesn't look right. It seems like this should go into the common parent, i.e., this should create just ./Bar.sv as that is the LCA of A/Bar.sv and B/Bar.sv.

[dtzSiFive]

Generally LGTM, some nits, and some pre-existing details I might want to chase down but nothing blocking.

Thanks for handling the extmodule prefix issue discussed!

[seldridge]

The @dtzSiFive test does now work. I've tested this complete PR on a number of internal designs and observed no change in output directories.