Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL] Put layer collateral in testbench dir #6741

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

seldridge
Copy link
Member

Change the LowerLayers pass to put generated modules inside the testbench directory if one is specified and a DUT is present. This is a stopgap measure that keeps layers aligned with existing CIRCT compilation of FIRRTL. This is intended to be replaced with a better FIRRTL ABI based on filelists in the future.

This is currently missing handling of instances instantiated by the layer. Those need to also get moved to the testbench area. I'll mark this non-draft once it's fully working.

prefix + "\n" +
"`define " + prefix)
->setAttr("output_file", outputFileAttr);
setOutputFile(builder.create<sv::VerbatimOp>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider using the emit::File and emit::Verbatim operations, they are active & supported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can switch over to these now that they are available. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to handle this in a cleanup pass if that's alright.

@uenoku
Copy link
Member

uenoku commented Feb 26, 2024

Does thin mean we cannot use layers for non-testbench parts at least until it's replaced with.a better ABI? Not having any actual use case for non-testbench but just curios.

@seldridge seldridge force-pushed the dev/seldridge/lower-layers-directories branch 2 times, most recently from effbce8 to 31ab823 Compare February 27, 2024 23:38
@seldridge seldridge marked this pull request as ready for review February 28, 2024 00:18
@seldridge seldridge requested a review from rwy7 February 28, 2024 00:19
@seldridge seldridge force-pushed the dev/seldridge/lower-layers-directories branch from 31ab823 to 4ae9ac5 Compare February 28, 2024 01:35
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a pass re:code mostly, didn't look super close the behavior -- if this does what's needed and especially if we're on the path towards changing it, LGTM!

lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp Outdated Show resolved Hide resolved
continue;
auto dir = anno.getMember<StringAttr>("dirname");
assert(dir && "invalid test bench annotation");
testBenchDir = dir.getValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last annotation wins works for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of scattered opinion on this type of thing throughout the passes. There is a stalled effort on making the annotations more first-class. The incompletely rolled out stop-gap is to have dedicated annotation "extractors" which get the information out of a might-exist annotation, while also checking that the annotation is correct. Cf.

LogicalResult circt::firrtl::extractDUT(const FModuleOp mod, FModuleOp &dut) {

There's a missing function/piece of infrastructure that you tell it what you want to pull out of an array of annotations and it does it and the validation.

All that said, moving towards first-class support where the annotations are actually validated in IR and not when used is much better.

lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp Outdated Show resolved Hide resolved
lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp Outdated Show resolved Hide resolved
@seldridge
Copy link
Member Author

I'll let CI pass, then rebase and land.

Fix a bug in LowerToHW where a '#hw.output_file' would be incorrectly set
on a module that already has one.  This could result in behavior where an
earlier pass sets up an output structure which is then changed by
LowerToHW due to the location of the module in the instance graph.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change LowerToHW to not count bound instances as being under the DUT.
This is done as a stopgap measure to align FIRRTL layers with the
existing, legacy directory structure that CIRCT uses when provided with
both a MarkDUTAnnotation and a TestBenchDirAnnotation. Namely, anything
instantiated by the DUT is put in the default output directory and
anything instantiated only by other users (Grand Central, Test Harness,
etc.) is allowed to move into one of the directories that those other
users handle.  With this change, this will no longer cause layer-derived
modules or modules instantiated under layers exclusively to be moved into
the main output directory.

This is intended to be replaced with filelists/manifests where the build
system driving CIRCT is responsible for moving things into separate
directories after CIRCT finishes.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Change the LowerLayers pass to put generated modules inside the testbench
directory if one is specified and a DUT is present.  This is a stopgap
measure that keeps layers aligned with existing CIRCT compilation of
FIRRTL.  This is intended to be replaced with a better FIRRTL ABI based on
filelists in the future.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Fix a bug involving interactions of Grand Central and layers output
directories.  When compiling a design that has a DUT and a testbench
directory, treat modules that are instantiated by Grand Central and under
a bound module (which is lowered from a layer) as belonging to Grand
Central and should be placed in its output directory.

This will be revisited in the future as the FIRRTL ABI moves more towards
filelists or manifests.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge force-pushed the dev/seldridge/lower-layers-directories branch from fb80c1b to 679737a Compare February 28, 2024 21:35
@seldridge seldridge merged commit 679737a into main Feb 28, 2024
2 checks passed
@seldridge seldridge deleted the dev/seldridge/lower-layers-directories branch February 28, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants