[FIRRTL] Output directory control for layers and modules #6971

rwy7 · 2024-04-30T19:03:11Z

Add output directory control for layers and firrtl.

For specifying the output directories:

Add a new annotation called "circt.OutputDirAnnotation", for specifying the output directory of modules
For layers, add syntax in the firrtl surface language for optionally specifying the output directory. We can't use an annotation here because, annotations can't target layers.

In the Lower-Layers Pass:

Place layer collateral (the bindfile, any layerblocks->modules) under the output directory of the layer, if there is any.
Stop outputting layer collateral under the testbench/views

Add a new pass, AssignOutputDirs, which will sink modules into the output directories they are instantiated from. This pass runs after lower-layers. In conjunction with the changes to the lower-layers pass, this means that modules which are only used under a particular layer will be sunk into that layer's output directory.

uenoku · 2024-05-01T01:46:41Z

What is an expected behavior when modules with different output dir annotations are deduplicated?

rwy7 · 2024-05-06T19:26:09Z

What is an expected behavior when modules with different output dir annotations are deduplicated?

Hey, thanks for taking a look! Two modules will dedup iff they have the same output directory annotation (modulo dirname canonicalization when constructing the the hw::OutputFileAttr). I've added a test for this here.

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

seldridge

The LCA algorithm comes across as more complicated than I think it should be. It may be easier to adopt a naive prefix LCA (as that is very understandable) or to go the direction of naive Euler Tour + RMQ (which should be recognizable and has room for performance improvements later if needed). This does appear to be doing a lot of LCA queries hence the RMQ formulation may make sense

Comments throughout.

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

include/circt/Dialect/FIRRTL/Passes.td

lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp

test/Dialect/FIRRTL/annotations.mlir

test/Dialect/FIRRTL/assign-output-dirs.mlir

seldridge · 2024-05-06T22:35:45Z

test/firtool/dedup-modules-with-output-dirs.fir

@@ -0,0 +1,179 @@
+; RUN: firtool --split-input-file %s | FileCheck %s


This test would be tighter as MLIR and running only on dedup. This allows for violations of initialization checking, i.e., the test can have modules that are basically empty. Alternatively, firtool Foo.fir -parse-only | circt-opt -firrtl-dedup gives you FIRRTL syntax for the test.

I think the idea is this is testing that firtool -- whatever its pipeline is -- has the expected (or at least consistent, as it's under test) behavior from a user's experience regarding interaction of dedup + output_file (in terms exposed to user, not when/how/where we set what attributes internally).

For example, as-is, moving the output-dir-assignment pass before dedup -- while not changing dedup's behavior -- would cause test failures (that a test narrowed to dedup would not catch). Specifically regarding the last test in this file (and lower-layers.fir test too, apparently).

And is this indeed the intended behavior / semantics for "output dir" information end-to-end?

Would be good to pin down what is really expected/promised by using these knobs/annotations -- to inform the behavior of dedup or anything else that alters instance hierarchy in a way that changes what-goes-where (inlining, for example -- what if you inline a module with an output_file attribute on it (especially modules that it was the only parent of)?).

How'd y'all frame the intent here -- best-effort output directives, providing users ability to get their desired output even if not specifically promised in a particular way re:pipeline happenstance (but they could use more annotations / etc/ to change what happens if they don't like it?).

Looking at this test again, this test is very verbose in order to get dedup to work. I am fine to have it be end-to-end. However, I would change it to not have so much logic in every module. Instead, the modules only need a don't touch'd wire (of the same size if you want them to dedup or a different size if not).

uenoku · 2024-05-07T07:38:33Z

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

+  if (auto moduleOp = dyn_cast<FModuleOp>(op)) {
+    moduleOp->setAttr("output_file", outputFile);
+    return success();
+  }


This silently drops attributes if (mistakenly) multiple output dir annotations are attached to the module. Can we check and emit an error?

seldridge

I think this looks fine.

I do realize that we made a mistake in the representation of the output directory structure and that this should be both: (1) in the FIRRTL spec and (2) sunk into the IR directly. This avoids the entirety of the OutputDirInfo which is having to be created to track information that would be better represented in a symbol table.

seldridge · 2024-06-07T15:42:45Z

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

+When an output directory isn't explicitly declared, then its parent directory is
+implicitly the default output directory. To explicitly declare that a
+directory's parent is the default output directory, use an empty string as the
+parent.


Is there any problem that this allows the description of arbitrary directed, cyclic graphs as opposed to only trees?

I don't think there's a problem with acyclic graphs in terms of the LCA computation, though it is slightly more complicated as it needs to consider depth and there may exist more than one "LCA".

Consider the following precedence graph:

tl;dr: This should probably require that this is a tree.

In a follow-on, it may be good to formalize this into the FIRRTL spec using a similar structure like the layer declarations to encode the explicit tree:

circuit Foo: directory Foo "foo/": directory Bar "bar/" directory Baz "baz/"

Later language in the PR calls this a "tree". Nit: it would be good to be consistent throughout comments and docs that this is a tree if it must be a tree and not a graph.

I'm not 100% sure I am interpreting this comment right

Is there any problem that this allows the description of arbitrary directed, cyclic graphs as opposed to only trees?

Regarding cycles, and multiple parents, there are checks in the AssignOutputDirs pass that ensure the precedence "graph/tree" is acyclic and that every directory has a single parent declaration.

Regarding multiple roots in the precedence graph/tree, that's not possible either, the graph/tree has a single root, the "default output directory".

Are you suggesting I rework the precedence annotations, so that it can only express a tree by construction? I am open to suggestions, but I chose this representation so that there wouldn't have to be a single monolithic annotation declaring all output directories in a single shot.

I will s/graph/tree/ in the PR for clarity.

lib/Dialect/FIRRTL/Export/FIREmitter.cpp

test/Dialect/FIRRTL/parse-basic.fir

seldridge · 2024-06-07T16:23:26Z

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

+      auto nameField = anno.getMember<StringAttr>("name");
+      if (!nameField)
+        return err() << "output directory declaration missing name";
+      if (nameField.empty())
+        return err() << "output directory name cannot be empty";
+      auto name = canonicalize(nameField);
+
+      auto parentField = anno.getMember<StringAttr>("parent");
+      if (!parentField)
+        return err() << "output directory declaration missing parent";
+      auto parent = canonicalize(parentField);


This code, coupled with the behavior of the canonicalize function are a bit weird. The checking of the inputs to canonicalize are done here to produce errors. Yet, this is then checked again by canonicalize only to return values which are not checked.

Also, the separation of canonicalize(StringRef) from canonicalize(StringAttr) isn't necessary given that only the latter is used in this file.

Several other better factorings exist:

Move the error generation logic into canonicalize, check the return on canoncalize and just exit.

Move only the checking into canonicalize and keep the error messages where they are. This requires combining the error messages of !nameField and nameField.empty().

Keep the current structure, but do not check in canonicalize. This may motivate making canonicalize a lambda so that it is co-located.

Making canonicalize a lambda may be better, too, given the single use and, for (1) or (2) the need to capture the circuit for the error message.

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

seldridge · 2024-06-07T17:26:15Z

lib/Dialect/HW/HWAttributes.cpp

+StringAttr OutputFileAttr::getDirectoryAttr() {
+  if (isDirectory())
+    return getFilename();
+
+  auto dir = getFilename().getValue();
+  for (unsigned i = 0, e = dir.size(); i < e; ++i) {
+    if (dir.ends_with(llvm::sys::path::get_separator()))
+      break;
+    dir = dir.drop_back();
+  }
+
+  if (dir.empty())
+    return nullptr;
+
+  return StringAttr::get(getContext(), dir);
+}


This seems like it is doing: llvm::sys::path::get_parent_path?

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

test/firtool/lower-layers.fir

seldridge · 2024-06-07T17:52:16Z

test/firtool/dedup-modules-with-output-dirs.fir

@@ -0,0 +1,179 @@
+; RUN: firtool --split-input-file %s | FileCheck %s


Looking at this test again, this test is very verbose in order to get dedup to work. I am fine to have it be end-to-end. However, I would change it to not have so much logic in every module. Instead, the modules only need a don't touch'd wire (of the same size if you want them to dedup or a different size if not).

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

darthscsi

This seems like we are adding lots of complexity and I'm not sure we really need it.
These seem obvious and would be a good stand-alone PR:
a) all relative paths should be relative to the output directory (-o flag)
b) Modules without outputs set go to the output directory
c) Layers without outputs set go to the output directory
d) Layers extracted to a module inherit the output directory of the layer.
e) Anything for which the user explicitly specifies a directory has that directory respected.

The key 2 question are:
a) whether modules are placed in directories based on the elaboration (instantiation) tree. If so, it seems the Least common ancestor is the natural place for that.
b) were public modules go. Do they move based on instantiation graph or always go in the output dir. What if they specify a location?

For a, saying LCA seems fine.
For b, either seems fine.
For both, I think anything which explicitly specifies a directory must have that respected.

What I think should be a non-goal is making directory nesting encode layer dependency despite what the user specifies for output directory of layers.

seldridge · 2024-06-07T19:41:17Z

☝️ After discussing with @darthscsi more about this. This comment is saying that we can remove the OutputDirPrecedenceAnnotation and any logic associated with it and instead compute the LCA based on the LCA of the directories themselves. E.g., SimpleLCA("verilog/design/", "verilog/testbench/") == "verilog/". This cannot express certain things like we do internally where AnnoyingLCA("verilog/design/", "verilog/testbench/") == "verilog/design/"). However, we can change this internally so that we can use the SimpleLCA computation.

Edit: We may be able to get the same behavior as we have today for SimpleLCA by always choosing the provided output directory ("verilog/design/") if the LCA is above the provided output directory.

This needs more discussion based on Lenharth feedback.

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

seldridge · 2024-06-11T20:32:33Z

lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp

+      auto &body = layer.getBody().getBlocks().front();
+      if (!body.empty()) {
+        auto begin = body.op_begin<LayerOp>();
+        auto end = body.op_end<LayerOp>();
+        path.emplace_back(FlatSymbolRefAttr::get(layer.getSymNameAttr()));
+        stack.emplace_back(begin, end);
+      }
+
+      while (!stack.empty() && idx(stack.back()) == end(stack.back())) {
+        stack.pop_back();
+        path.pop_back();
+      }
+
+      if (stack.empty())
+        break;
+
+      assert(idx(stack.back()) != end(stack.back()));
+      layer = *idx(stack.back());
+      ++idx(stack.back());


This looks like overkill. Can't this just lookup the layer by symbol and see if it has an output file attribute when it needs it? Alternatively, this could be cached, though isn't that just a walk over LayerOp?

seldridge

LGTM

Thanks for the iteration on this, Rob. I think you've arrived at an excellent, minimal, and understandable design point. 💯

seldridge · 2024-06-13T02:54:56Z

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

+  fs::make_absolute(outputDir, moduleOutputDir);
+  path::remove_dots(moduleOutputDir, true);


This looks to be the correct way to canonicalize this. 👍 It's kind of surprising that there isn't an llvm::canonicalizePath which doesn't try to use the discovered current directory as the actual current directory.

llvm::sys::fs::real_path could be a one-stop-shop for this if we ever wanted tilde expansion and symlink resolution, too.

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp

test/Dialect/FIRRTL/annotations-errors.mlir

seldridge · 2024-06-13T03:36:51Z

test/firtool/lower-layers-directories.fir

+; CHECK-DAG: FILE "verification{{[/\]}}testbench{{[/\]}}layers_Testbench_A.sv"
+; CHECK-DAG: FILE "verification{{[/\]}}testbench{{[/\]}}Bar.sv"
+; CHECK-DAG: FILE "verification{{[/\]}}testbench{{[/\]}}DUT_A.sv"
+; CHECK-DAG: FILE "verification{{[/\]}}testbench{{[/\]}}Testbench.sv"


This nicely matches the spirit of the original test.

seldridge · 2024-06-13T03:41:00Z

test/Dialect/FIRRTL/assign-output-dirs.mlir

+  firrtl.module @InA() attributes {output_file = #hw.output_file<"A/foo">} {
+    firrtl.instance ra @ByRA()
+    firrtl.instance ab @ByAB()
+    firrtl.instance a  @ByA()
+    firrtl.instance ac @ByAC()
+  }
+
+  firrtl.module @InB() attributes {output_file = #hw.output_file<"B/foo">} {
+    firrtl.instance ab @ByAB()
+    firrtl.instance bc @ByBC()
+  }


This is a great edge case to test!

This helper gets the directory component of an output file name, or returns nullptr if there is none.

Instead of using an explicit precedence declaration anno to help guide the assignment of floating modules to output directories, use the directory hierarchy itself. So if a module is used under directory A/B and A/C, it will be placed into directory A.

dtzSiFive

Thanks for pushing on this!!! Small feedback, generally LGTM! 🎉

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp

rwy7 · 2024-06-14T18:51:56Z

Thanks everyone!

rwy7 force-pushed the output-control branch from 983d40c to c6899d4 Compare May 3, 2024 16:16

rwy7 marked this pull request as ready for review May 6, 2024 18:53

rwy7 requested review from darthscsi, dtzSiFive and seldridge as code owners May 6, 2024 18:53

rwy7 force-pushed the output-control branch from 0549e9d to a4e35cc Compare May 6, 2024 19:24

dtzSiFive reviewed May 6, 2024

View reviewed changes

docs/Dialects/FIRRTL/FIRRTLAnnotations.md Outdated Show resolved Hide resolved

rwy7 force-pushed the output-control branch from dcc6aac to 19b566a Compare May 6, 2024 21:22

seldridge reviewed May 6, 2024

View reviewed changes

uenoku reviewed May 7, 2024

View reviewed changes

rwy7 force-pushed the output-control branch from bcc1b8c to 9a8c66a Compare May 22, 2024 18:10

rwy7 force-pushed the output-control branch from e3fd7ca to 3b18e7d Compare June 7, 2024 15:26

seldridge previously approved these changes Jun 7, 2024

View reviewed changes

darthscsi reviewed Jun 7, 2024

View reviewed changes

docs/Dialects/FIRRTL/FIRRTLAnnotations.md Outdated Show resolved Hide resolved

darthscsi reviewed Jun 7, 2024

View reviewed changes

docs/Dialects/FIRRTL/FIRRTLAnnotations.md Outdated Show resolved Hide resolved

darthscsi reviewed Jun 7, 2024

View reviewed changes

rwy7 force-pushed the output-control branch 2 times, most recently from 9a329db to ea8443c Compare June 11, 2024 18:51

seldridge reviewed Jun 11, 2024

View reviewed changes

seldridge approved these changes Jun 13, 2024

View reviewed changes

rwy7 force-pushed the output-control branch from 9708b93 to c257b7c Compare June 13, 2024 19:28

rwy7 added 5 commits June 14, 2024 12:21

Add getDirectoryAttr helper to HWOutputFileAttr

caf8a67

This helper gets the directory component of an output file name, or returns nullptr if there is none.

Output directory control v2

166a725

Instead of using an explicit precedence declaration anno to help guide the assignment of floating modules to output directories, use the directory hierarchy itself. So if a module is used under directory A/B and A/C, it will be placed into directory A.

Support absolute output directories for modules

13f3087

Add comment

46a8acc

Make it so output dir annos only apply to public modules

ce921a0

rwy7 added 7 commits June 14, 2024 12:21

Simplify lower layers

01264fc

Add ability to configure the output directory of assign-output-dirs

81bb2b8

Update tests

a0d6aed

Address review comments

3b2c3d9

Clean up whitespace in test

8dedb53

clang-format

89d0b11

Fix up firtool integration test excercising dedup + output dirs

ccdb32c

rwy7 force-pushed the output-control branch from 50ddd67 to ccdb32c Compare June 14, 2024 16:21

dtzSiFive approved these changes Jun 14, 2024

View reviewed changes

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp Outdated Show resolved Hide resolved

lib/Dialect/FIRRTL/Transforms/AssignOutputDirs.cpp Outdated Show resolved Hide resolved

Address review comments

73affae

rwy7 merged commit 41ebd04 into llvm:main Jun 14, 2024
4 checks passed

rwy7 deleted the output-control branch June 14, 2024 18:51

rwy7 mentioned this pull request Jun 14, 2024

Fix paths in tests for windows builds #7185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIRRTL] Output directory control for layers and modules #6971

[FIRRTL] Output directory control for layers and modules #6971

rwy7 commented Apr 30, 2024 •

edited

Loading

uenoku commented May 1, 2024

rwy7 commented May 6, 2024

seldridge left a comment

seldridge May 6, 2024

dtzSiFive May 7, 2024

seldridge Jun 7, 2024

uenoku May 7, 2024

seldridge left a comment

seldridge Jun 7, 2024

seldridge Jun 7, 2024

rwy7 Jun 7, 2024

seldridge Jun 7, 2024

seldridge Jun 7, 2024 •

edited

Loading

seldridge Jun 7, 2024

darthscsi left a comment •

edited

Loading

seldridge commented Jun 7, 2024 •

edited

Loading

seldridge Jun 11, 2024

seldridge left a comment

seldridge Jun 13, 2024

seldridge Jun 13, 2024

seldridge Jun 13, 2024

dtzSiFive left a comment

rwy7 commented Jun 14, 2024

		@@ -0,0 +1,179 @@
		; RUN: firtool --split-input-file %s \| FileCheck %s

		fs::make_absolute(outputDir, moduleOutputDir);
		path::remove_dots(moduleOutputDir, true);

[FIRRTL] Output directory control for layers and modules #6971

[FIRRTL] Output directory control for layers and modules #6971

Conversation

rwy7 commented Apr 30, 2024 • edited Loading

uenoku commented May 1, 2024

rwy7 commented May 6, 2024

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthscsi left a comment • edited Loading

Choose a reason for hiding this comment

seldridge commented Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtzSiFive left a comment

Choose a reason for hiding this comment

rwy7 commented Jun 14, 2024

rwy7 commented Apr 30, 2024 •

edited

Loading

seldridge Jun 7, 2024 •

edited

Loading

darthscsi left a comment •

edited

Loading

seldridge commented Jun 7, 2024 •

edited

Loading