[FIRRTL] Emit a bindfile for each public module #8458
Conversation
There was a problem hiding this comment.
This all looks great!
One test I was thinking of is around the behavior of the includes and how that composes with directories. Consider the following:
FIRRTL version 5.1.0
circuit Foo: %[[
{"class": "firrtl.transforms.DontTouchAnnotation", "target": "~|Foo>b"},
{"class": "firrtl.transforms.DontTouchAnnotation", "target": "~|Bar>a"}
]]
layer A, bind, "A":
layer B, bind, "B":
public module Bar enablelayer A:
layerblock A:
node a = UInt<1>(0)
public module Foo enablelayer A:
inst bar of Bar
layerblock B:
node b = UInt<1>(0)With this PR, this produces the following bind files:
// ----- 8< ----- FILE "A/layers-Bar-A.sv" ----- 8< -----
// Generated by CIRCT firtool-1.116.0-4-g651c02833
`ifndef layers_Bar_A
`define layers_Bar_A
bind Bar Bar_A a ();
`endif // not def layers_Bar_A
// ----- 8< ----- FILE "B/layers-Bar-B.sv" ----- 8< -----
// Generated by CIRCT firtool-1.116.0-4-g651c02833
`ifndef layers_Bar_B
`define layers_Bar_B
`endif // not def layers_Bar_B
// ----- 8< ----- FILE "A/layers-Foo-A.sv" ----- 8< -----
// Generated by CIRCT firtool-1.116.0-4-g651c02833
`ifndef layers_Foo_A
`define layers_Foo_A
`include "layers-Bar-A.sv"
`endif // not def layers_Foo_A
// ----- 8< ----- FILE "B/layers-Foo-B.sv" ----- 8< -----
// Generated by CIRCT firtool-1.116.0-4-g651c02833
`ifndef layers_Foo_B
`define layers_Foo_B
`include "layers-Bar-B.sv"
bind Foo Foo_B b ();
`endif // not def layers_Foo_BIt would be better if any include was relative to the output directory, i.e.:
`include "A/layers-Bar-A.sv"
`include "B/layers-Bar-B.sv"
My reasoning here is that keeping this all relative to one directory (the output directory) simplifies Verilog tooling as you only have to tell the tool about one include directory. If, instead, this does what it is doing today, then you have to tell the tool about all layer directories in order for this to work.
Specifically, we're using item one of the Verilog spec here and it's convenient to keep this to either "the compiler's current working directory" (the user has to do nothing) or exactly one "user-specified location". Most simulators support the +incdir option which sets additional user-specified locations.
| @@ -57,7 +60,6 @@ enum class Delimiter { BindModule = '_', BindFile = '-', InlineMacro = '$' }; | |||
| /// use to create names in the global namespace. This is allocated per-layer | |||
| /// block. | |||
| struct LayerBlockGlobals { | |||
|
|
|||
There was a problem hiding this comment.
Nit: unrelated change. Feel free to land separately.
| instanceName, NameKindEnum::DroppableName, | ||
| instanceName, NameKindEnum::InterestingName, |
There was a problem hiding this comment.
Was this change made due to observable changes in the output?
I'm asking primarily because InterestingName comes from a Chisel name that might be useful to a user if we don't drop it. However, the layer blocks themselves, don't have defined Chisel-observable names nor are they actually important ABI-wise. Put differently, is it possible for a compiler-generated module to have an interesting name?
More pragmatically, I don't think this has any affect at this point in the pipeline other than causing LowerToHW to create a public inner symbol for any operation which still has an interesting name. Yet, this is already creating an inner symbol on the instance.
There was a problem hiding this comment.
I've dropped it! I'm not sure why I did it this way, I don't need it to be an interesting name. Thanks.
| auto outputFile = | ||
| outputFileForLayer(circuitName, layerBlock.getLayerName()); | ||
| /*portAnnotations=*/ArrayRef<Attribute>{}, /*lowerToBind=*/false, | ||
| /*doNotPrint=*/true, innerSym); |
There was a problem hiding this comment.
I'm still a little hazy on the story around lowerToBind vs. doNotPrint. This works because lowerToBind is replaced with bind ops in the bind files, correct?
Regardless, having both of these is not ideal and would be good to clean up.
|
|
||
| // ----- | ||
|
|
||
| // Check that for private modules, bindfiles files are only generated if they are used. |
| ; CHECK-LABEL: FILE "layers-Foo-A-B.sv" | ||
| ; CHECK: `include "layers-Foo-A.sv" | ||
| ; CHECK-NEXT: `ifndef layers_Foo_A_B | ||
| ; CHECK-NEXT: `define layers_Foo_A_B | ||
| ; CHECK-NEXT: bind Foo Foo_A_B a_b (); | ||
| ; CHECK-NEXT: `endif // layers_Foo_A_B | ||
|
|
||
| ; CHECK-LABEL: FILE "layers-Foo-A.sv" | ||
| ; CHECK: `ifndef layers_Foo_A | ||
| ; CHECK-NEXT: `define layers_Foo_A | ||
| ; CHECK-NEXT: bind Foo Foo_A a (); | ||
| ; CHECK-NEXT: `endif // layers_Foo_A | ||
| ; CHECK-NEXT: `define layers_Foo_A | ||
| ; CHECK-NEXT: bind Foo Foo_A a (); | ||
| ; CHECK-NEXT: `endif // not def layers_Foo_A | ||
|
|
||
| ; CHECK-LABEL: FILE "layers-Foo-A-B.sv" | ||
| ; CHECK: `ifndef layers_Foo_A_B | ||
| ; CHECK-NEXT: `define layers_Foo_A_B | ||
| ; CHECK-NEXT: `include "layers-Foo-A.sv" | ||
| ; CHECK-NEXT: bind Foo Foo_A_B a_b (); | ||
| ; CHECK-NEXT: `endif // not def layers_Foo_A_B |
There was a problem hiding this comment.
This re-ordering to layer order is quite nice.
3267d88 to
995e660
Compare
|
I've updated this PR so that, when a bindfile includes another, the inclusion uses the includee's output path. There is a test, too. Here is the output: // Generated by CIRCT unknown git version
module Bar();
endmodule
module Foo();
Bar bar ();
endmodule
// ----- 8< ----- FILE "A/Bar_A.sv" ----- 8< -----
// Generated by CIRCT unknown git version
module Bar_A();
wire a = 1'h0;
endmodule
// ----- 8< ----- FILE "A/layers-Bar-A.sv" ----- 8< -----
// Generated by CIRCT unknown git version
`ifndef layers_Bar_A
`define layers_Bar_A
bind Bar Bar_A a ();
`endif // not def layers_Bar_A
// ----- 8< ----- FILE "B/layers-Bar-B.sv" ----- 8< -----
// Generated by CIRCT unknown git version
`ifndef layers_Bar_B
`define layers_Bar_B
`endif // not def layers_Bar_B
// ----- 8< ----- FILE "B/Foo_B.sv" ----- 8< -----
// Generated by CIRCT unknown git version
module Foo_B();
wire b = 1'h0;
endmodule
// ----- 8< ----- FILE "A/layers-Foo-A.sv" ----- 8< -----
// Generated by CIRCT unknown git version
`ifndef layers_Foo_A
`define layers_Foo_A
`include "A/layers-Bar-A.sv"
`endif // not def layers_Foo_A
// ----- 8< ----- FILE "B/layers-Foo-B.sv" ----- 8< -----
// Generated by CIRCT unknown git version
`ifndef layers_Foo_B
`define layers_Foo_B
`include "B/layers-Bar-B.sv"
bind Foo Foo_B b ();
`endif // not def layers_Foo_B |
For each public module, emit a bind file for every layer we know about, regardless of whether the layer has actually been used under the module. For each private module, emit a bind file for every layer used under that module, whether directly referenced via a layerblock op, or indirectly through an instance op. Do not emit a bind file for a layer, if the layer is not used. For a bind file emitted for a module M and layer L, the bind file should automatically enable any parent layers of L. If L has a parent layer L', and L' is a bind layer, include the bind file for M/L'. M is guaranteed to have a bind file for L'. If L' is a bind layer, define the macro for L' and enable any parents of L'. The bind file should also enable the layer globally. If M instantiates a module M', include the bind file for M'/L, if it exists. Use structured sv ops to build the bind file contents.
This reverts a suggestion [[1]] I made on #8458. This changes the (undocumented) ABI of FIRRTL layers to _not_ include any directory information in the `` `include `` directives used to enable layers. The reason for this is that using the full paths (relative to the root output directory) prevents rearranaging directories after compilation. The downside to this is that users must include more `+incdir` command line options to include _all_ directories in a design. However, I think this is reasonable. I do think that we can roll this back to use the original behavior in the future in a backwards-compatible way. I.e., making the change from not including the full path to including the full path won't break any downstream tools as these will just be including _too many_ `+incdir`s. [1]: #8458 (review) Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
This reverts a suggestion [[1]] I made on #8458. This changes the (undocumented) ABI of FIRRTL layers to _not_ include any directory information in the `` `include `` directives used to enable layers. The reason for this is that using the full paths (relative to the root output directory) prevents rearranaging directories after compilation. The downside to this is that users must include more `+incdir` command line options to include _all_ directories in a design. However, I think this is reasonable. I do think that we can roll this back to use the original behavior in the future in a backwards-compatible way. I.e., making the change from not including the full path to including the full path won't break any downstream tools as these will just be including _too many_ `+incdir`s. [1]: #8458 (review) Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
According to the FIRRTL ABI, we should be emitting a bindfile for each layer × public-module combination. Currently, we only emit one bindfile per layer, which enables the layer for the entire circuit.
This PR updates lower layers to correctly implement the FIRRTL ABI by emitting bindfiles for each public module.