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

Add RTLIL Backend. #2331

Merged
merged 5 commits into from
Sep 29, 2021
Merged

Add RTLIL Backend. #2331

merged 5 commits into from
Sep 29, 2021

Conversation

nxmq
Copy link
Contributor

@nxmq nxmq commented Aug 18, 2021

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you update the FIRRTL spec to include every new feature/behavior?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • backend code generation
  • new feature/API

API Impact

This PR adds the names "experimental-rtlil" and "rtlil" to the list of valid emitters, and also adds the annotations required to utilize the emitter. These are the only changes to the public-facing API.

Backend Code Generation Impact

This PR introduces an entirely new code generation backend, targeting the textual representation of the RTLIL IR format used by the Yosys open synthesis tool. As such, it should have minimal effects on the existing Verilog backend, as the only change made which could affect that backend was making verilog-specific memory lowering an optional dependency of the CombineCats pass, instead of a required one.

Desired Merge Strategy

Rebase please.

Release Notes

  • Adds code generation backend targeting RTLIL IR for use with the Yosys open synthesis tool.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

Copy link
Contributor

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

This looks awesome. I added some small initial feedback.
Before this gets merged we need some tests. You can look at https://github.com/chipsalliance/firrtl/tree/master/src/test/scala/firrtl/backends/experimental/smt for inspiration.

A minimal smoke test could be to run your backend on all our regression firrtl files and then to just read the result with yosys to make sure it is at least valid RTLIL. Like: https://github.com/chipsalliance/firrtl/blob/master/src/test/scala/firrtl/backends/experimental/smt/end2end/SMTCompilationTest.scala

Thanks!

src/main/scala/firrtl/Emitter.scala Outdated Show resolved Hide resolved
src/main/scala/firrtl/Emitter.scala Outdated Show resolved Hide resolved
src/main/scala/firrtl/stage/FirrtlAnnotations.scala Outdated Show resolved Hide resolved
src/main/scala/firrtl/transforms/CombineCats.scala Outdated Show resolved Hide resolved
@nxmq
Copy link
Contributor Author

nxmq commented Aug 18, 2021

This looks awesome. I added some small initial feedback.
Before this gets merged we need some tests. You can look at https://github.com/chipsalliance/firrtl/tree/master/src/test/scala/firrtl/backends/experimental/smt for inspiration.

A minimal smoke test could be to run your backend on all our regression firrtl files and then to just read the result with yosys to make sure it is at least valid RTLIL. Like: https://github.com/chipsalliance/firrtl/blob/master/src/test/scala/firrtl/backends/experimental/smt/end2end/SMTCompilationTest.scala

Thanks!

One test I did use throughout development was using yosys's equiv_check and related features to verify equivalence between the circuit as emitted by the rtlil backend and the circuit as emitted by the verilog backend. Unfortunately, this test takes about 5m to run on even a moderately complex circuit, so I feel hesitant about including something with such a long runtime into the tests for firrtl. Should I add an equiv_check based test, or a simple "does yosys consume this" test?

@ekiwi
Copy link
Contributor

ekiwi commented Aug 18, 2021

One test I did use throughout development was using yosys's equiv_check and related features to verify equivalence between the circuit as emitted by the rtlil backend and the circuit as emitted by the verilog backend. Unfortunately, this test takes about 5m to run on even a moderately complex circuit, so I feel hesitant about including something with such a long runtime into the tests for firrtl. Should I add an equiv_check based test, or a simple "does yosys consume this" test?

I would go for "does yosys consume this" for all regression designs and then a equiv_check for a very small design.

@ekiwi
Copy link
Contributor

ekiwi commented Aug 18, 2021

To solve the CI issues, you just need to run:

sbt scalafmtAll

@ekiwi
Copy link
Contributor

ekiwi commented Aug 18, 2021

Not saying you need to do this to get the PR merged, but just as another idea on how we could test this backend:
You could use cxxrtl to generate an executable simulation with yosys and then you could write some executable unittests similar to what we do with Verilator to test the Verilog emission.

@nxmq nxmq changed the title Added RTLIL Backend. Add RTLIL Backend. Aug 18, 2021
@ekiwi
Copy link
Contributor

ekiwi commented Aug 19, 2021

Looks like some of your code doesn't compile on scala 2.13. To debug that you can do sbt ++2.13.4 compile

@ekiwi
Copy link
Contributor

ekiwi commented Aug 23, 2021

@nxmq We discussed this PR in our dev meeting and we are happy to merge it once the tests pass.

I assume that the reason the CI fails right now is that we might be using an older version of yosys. Could you add some output to stdout to debug what is going wrong with yosys?

@ekiwi
Copy link
Contributor

ekiwi commented Aug 23, 2021

Could you also add some of your formal equivalence check tests? Maybe marked as ignore for now? We would be interested in running them from time to time (but maybe not on every PR).

@nxmq
Copy link
Contributor Author

nxmq commented Aug 25, 2021

@nxmq We discussed this PR in our dev meeting and we are happy to merge it once the tests pass.

I assume that the reason the CI fails right now is that we might be using an older version of yosys. Could you add some output to stdout to debug what is going wrong with yosys?

The failures do seem to be due to the old version of yosys used during testing, my backend as is only supports versions of yosys released in the last 6 months or so. I can def pass some debug output through for showing whats wrong.

@ekiwi
Copy link
Contributor

ekiwi commented Aug 25, 2021

The failures do seem to be due to the old version of yosys used during testing, my backend as is only supports versions of yosys released in the last 6 months or so. I can def pass some debug output through for showing whats wrong.

Do you have a good source of yosys binaries that you can recommend or are you just building from the latest master? Yosys has not had a release in about two years now, so that makes it hard to decide which "version" to use.

@nxmq
Copy link
Contributor Author

nxmq commented Aug 27, 2021

Could you also add some of your formal equivalence check tests? Maybe marked as ignore for now? We would be interested in running them from time to time (but maybe not on every PR).

Sure, I should be able to add a few of those test circuits soon. As for yosys binary sources, currently I just build master.

@ekiwi
Copy link
Contributor

ekiwi commented Sep 23, 2021

This PR might help us get a newer yosys version: #2365

Copy link
Contributor

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

Two small changes and then I think this new experimental backend is ready to 🚢

src/main/scala/firrtl/Emitter.scala Outdated Show resolved Hide resolved
case "mverilog" => new MinimumVerilogEmitter
case "sverilog" => new SystemVerilogEmitter
case "experimental-rtlil" => new RtlilEmitter
case _ => throw new OptionsException(s"Unknown compiler name '$a'! (Did you misspell it?)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo your changes to this file. This code is for the (soon to be) deprecated -X option. New emitters should only use the -E option.

@ekiwi ekiwi added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Sep 29, 2021
Copy link
Contributor

@ekiwi ekiwi left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you very much for your patience and your hard work!

Looks like this change will make it into the upcoming release and it should be part of the snapshot in 1-2h. I would be happy to review any followup PRs if you see that something is missing or anything needs to be fixed.

@mergify mergify bot merged commit e70ee53 into chipsalliance:master Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants