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][NFC] Add fir tests relating to probes, from spec. #5781

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

dtzSiFive
Copy link
Contributor

Examples tweaked as-needed to make full examples,
portion from spec are marked as well as any modifications.

See comments in each, a few are modified to only test the demonstrated feature or to workaround spec issues (notably can't express non-const node with literal initializer).

Check these are parsed and make it through the pipeline, with and without aggregate preservation options.

Test invalid or unsupported examples are appropriately rejected.

Ensure the examples from spec are under test.

@dtzSiFive dtzSiFive added the FIRRTL Involving the `firrtl` dialect label Aug 4, 2023
@dtzSiFive dtzSiFive changed the title [FIRRTL] Add fir tests relating to probes, from spec. [FIRRTL][NFC] Add fir tests relating to probes, from spec. Aug 4, 2023
Examples tweaked as-needed to make full examples,
portion from spec are marked as well as any modifications.

See comments in each, a few are modified to only test
the demonstrated feature or to workaround spec issues
(notably can't express non-const node with literal initializer).

Check these are parsed and make it through the pipeline,
with and without aggregate preservation options.

Test invalid or unsupported examples are appropriately
rejected.

Ensure the examples from spec are under test.
@dtzSiFive
Copy link
Contributor Author

These probably should be integration tests (similarly other files in test/firtool) and ideally would do more than just run firtool on them, but it's a start and helps ensure pass composition keeps these things working.

@dtzSiFive dtzSiFive merged commit d79d79d into llvm:main Aug 8, 2023
5 checks passed
@dtzSiFive dtzSiFive deleted the feature/reftests branch August 8, 2023 21:33
@seldridge
Copy link
Member

seldridge commented Aug 9, 2023

Great! Do you have any ideas on how to keep these from getting out-of-date?

At the risk of over-engineering this... it would be interesting if the spec examples were automatically all tested and kept in sync. This would also force the spec to only include legal FIRRTL (possibly at the cost of readability due to checks like initialization coverage and always having to use circuit and module boilerplate).

One idea would be to embed FileCheck inside Markdown comments in the spec. This impacts spec source readability. Another idea would be to include the FileCheck intermixed in every FIRRTL example and then rip it out with sed or a pandoc filter.

@dtzSiFive
Copy link
Contributor Author

That's a really good idea, I'd like that quite a lot too.
Since didn't have a better plan but wanted that sort of thing, when writing the ref spec for FIRRTL I early on pulled all examples out so I could sanity-check them and such: https://github.com/dtzSiFive/reftype-inputs (spec subdirectory, repo is rougher than I'd like, was going to have a reftype-tester for all that but think that is better in-tree, since this PR).

I like your idea of some pipeline (sed or pandoc filter) to allow us to have "real" examples without impacting readability/terseness of the examples in the specification-- maybe could extract the "spec" portions from a larger example (not just drop FileCheck directives), using known markers not unlike the "SPEC BEGIN" / "SPEC END" I used here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants