[FIRRTL] Output directory control for layers and modules #6971
Conversation
|
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. |
| } | ||
| ``` | ||
|
|
||
| ### DeclareOutputDirAnnotation |
There was a problem hiding this comment.
Consider renaming this -- this is not for declaring anything, it's for specifying the parent.
Besides clarity, specifically since you /can't/ emit one of these for all directories (those without a parent?), a different name might be a good idea.
There was a problem hiding this comment.
I really don't like the name! Please, suggest something :)
OutputDirPrecedenceAnnotation?
Although just as a note, you actually can declare a directory whose parent is "no output directory", by using an empty string as the parent name. It's just that there is no need.
There was a problem hiding this comment.
Interpreting empty string as default output directory (and a valid entry) is unexpected to me (if this is important, consider mentioning it? Maybe omit the parent entry instead? 🤷 )! But that makes sense.
Names ARE hard, haha, I'll circle back 👍.
Your suggestion is an improvement IMO 😄 .
There was a problem hiding this comment.
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.
| "name": "verification_extras", | ||
| "parent": "verification" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Can this include documentation of how to specify that the parent is the root? Is this an empty string, optional JSON field, or null?
| hw::OutputFileAttr bindFile; | ||
| if (auto outputFile = | ||
| layerOp->getAttrOfType<hw::OutputFileAttr>("output_file")) { | ||
| auto dir = outputFile.getDirectoryAttr().getValue(); | ||
| bindFile = hw::OutputFileAttr::getFromDirectoryAndFilename( | ||
| &getContext(), dir, prefix + ".sv", | ||
| /*excludeFromFileList=*/true); | ||
| } else { | ||
| bindFile = hw::OutputFileAttr::getFromFilename( | ||
| &getContext(), prefix + ".sv", /*excludeFromFileList=*/true); | ||
| } |
There was a problem hiding this comment.
Having an empty auto dir might make this cleaner as both code paths can use getFromDirectoryAndFilename.
| rawAnnotations = [ | ||
| { | ||
| class = "circt.OutputDirAnnotation", | ||
| dirname = "foobarbaz", |
There was a problem hiding this comment.
Two more cases may be interesting to test:
foobarbaz/foobarbaz/qux
| ] | ||
| } { | ||
|
|
||
| firrtl.module private @AssignOutputDirs() {} |
There was a problem hiding this comment.
I'm surprised CIRCT accepts this as I thought the main module had to be public.
| // when a module with an output directory is used, that | ||
| // directory is respected | ||
|
|
||
| // when a module doesnt have an output directory, it's dragged | ||
|
|
||
| // when a module is used in both a child and parent, it is dragged | ||
|
|
||
| // when a module is used by two children, it is dragged to parent. | ||
|
|
||
| // circular parent/child relationship | ||
| // parent is null / root | ||
| // child name is empty??? |
There was a problem hiding this comment.
Nit: strip comments and add file trailing newline.
| @@ -0,0 +1,179 @@ | |||
| ; RUN: firtool --split-input-file %s | FileCheck %s | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?).
| if (auto moduleOp = dyn_cast<FModuleOp>(op)) { | ||
| moduleOp->setAttr("output_file", outputFile); | ||
| return success(); | ||
| } |
There was a problem hiding this comment.
This silently drops attributes if (mistakenly) multiple output dir annotations are attached to the module. Can we check and emit an error?
This helper gets the directory component of an output file name, or returns nullptr if there is none.
This pass "drags" modules into directories where they are instantiated.
Layers can't be targeted by annotations, so we add this as an attribute to layers directly, and parse the output directory directly too.
Use the output directory of a layer as the output directory for it's assets, including: - extracted layerblock modules - the bindfile The assign-output-dirs pass will sink additional modules into layer-specific output directories, if possible.
Had some bad copy-pasting.
Check that if two children modules are deduped, that the resulting single module is placed in the LCA directory of all users.
Add output directory control for layers and firrtl.
For specifying the output directories:
In the Lower-Layers Pass:
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.