git-subtree add sifive/chisel-circt #2477

seldridge · 2022-04-06T15:51:19Z

This commit is the first step towards enabling CIRCT-compiled Chisel natively inside Chisel (as opposed to relying on a separate library).

This does three things:

It uses git-subtree to add https://github.com/sifive/chisel-circt (with history) as a directory called circt/.
It moves and updates the contents of circt/ into src/main/scala/circt/ or src/test/scala/circt/.
It removes unnecessary files (build.sbt/build.sc) from the circt tree and updates the README to reference the fact that this is no longer a separate dependency.

This PR is expected to fail until we have a way of adding CIRCT as a dependency to the CI.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add CI

Add a "handover" option that augments circt.stage.ChiselStage to lower using a combination of the Scala FIRRTL Compiler (SFC) followed by CIRCT. The handover can be set at CHIRRTL (don't run the SFC), High FIRRTL, Middle FIRRTL, Low FIRRTL, or Optimized Low FIRRTL. Include tests of this behavior by looking at the FirrtlCircuitAnnotation that exists before running CIRCT for different handover values. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add --handover option to lower partially using SFC

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add publishing to sonatype

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

- Add Maven release, Sonatype snapshots, and Javadoc.io badges - Improve setup instructions Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add an option to output using a "*.v" suffix. This is still using the CIRCT SystemVerilog back end, though. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Change command line option passed to "firtool" due to upstream change. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Change "RTL" to "HW"

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

* add mill build system * bump to Chisel 3.5.0 * use cross build to select Scala version. * add gitignore * Add/use Compiler Plugin

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add Scala 2.13, enable 2.13 CI

Change the default handover from optimized low FIRRTL to CHIRRTL. This is done because CIRCT is an essentially complete FIRRTL compiler now. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Change ChiselStage to pass annotations to firtool via a file. Also, add logic to remove lots of annotations and convert them to equivalent firtool options. Any annotations that we don't know what to do with are passed on to firtool to let it deal with them. This has the effect of both communicating things like DontTouchAnnotation and also providing information to the user that they are passing annotations that firtool doesn't know how to handle. To do the latter checking "-warn-on-unprocessed-annotations" is turned on. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add numerous tests that are checking the behavior of annotation passing and consumption by CIRCT. This adds tests for: - InlineInstance - FlattenInstance - dontTouch (of wires and nodes) - forceName (of modules only) - Deduplication and doNotDedup Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add Annotation support and conversion to firtool CLI options.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add an annotation, FirtoolOption, that can be used to pass a command line option directly to firtool. More than one FirtoolOption can be specified. Add a test that shows using this to enable case statement emission through a "--lowering-option". Fixes #12. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

ekiwi · 2022-04-06T16:19:23Z

Should this be in an experimental package or otherwise marked as not meant for general usage until stable binary releases for firtool are available for Mac and Linux?

seldridge · 2022-04-06T17:54:10Z

Good point, Kevin. My plan is to block this PR (keep it as a draft) until we have binary releases available for CIRCT.

ekiwi · 2022-04-06T18:49:50Z

Good point, Kevin. My plan is to block this PR (keep it as a draft) until we have binary releases available for CIRCT.

Sounds good. Do you think it would be feasible to have some code that automatically determines the platform we are on and then downloads the latest firtool release (or a compatible version) for the current platform from github?
That way it would be just as easy as using the old firrtl compiler.

* Add scalafmt files from chisel3 * Enable scalafmt in SBT * Scalafmt all the files * Add scalafmt check to CI

Add support for the new, finer-grained aggregate preservation options that were added to CIRCT/firtool. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

mwachs5 · 2022-08-03T17:21:48Z

firtool binaries is now published on each release of CIRCT, so I plan to resurrect this PR

CLAassistant · 2022-08-04T05:15:07Z

All committers have signed the CLA.

seldridge · 2022-08-04T05:22:02Z

firtool binaries is now published on each release of CIRCT, so I plan to resurrect this PR

Specifically, this was added in llvm/circt#3617 and is automatically published as part of the release assets for CIRCT. E.g., the SiFive 1.11.0 release of CIRCT binaries are available here: https://github.com/llvm/circt/releases/tag/sifive%2F1%2F11%2F0 This is also what I'm using for CI in this PR.

Change GitHub CI to use a tagged, published Ubuntu binary of CIRCT instead of relying on a nightly docker image. This is both: (1) more stable and (2) way faster. This also removes a barrier to upstreaming chisel-circt with chisel3. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

seldridge · 2022-08-04T06:18:12Z

Logistically, any feedback on this PR that actually requests code modifications I'll address on upstream chisel-circt and then fold into this PR. I'd like to keep this PR as a pure: (1) git subtree add, (2) mv circt/ src/main/scala/, (3) enable CI, and (4) tag a final release of chisel-circt and archive it. If there are more nitty concerns with the code that can wait, I'm signing myself up to fix these after this lands.

mwachs5 · 2022-08-04T06:42:36Z

ok, so you would prefer to merge outstanding chisel-circt PRs first before merging into chisel3?

EDIT i misread the comment. Does this PR currently include at least the scalafmt change?

seldridge · 2022-08-04T17:04:17Z

ok, so you would prefer to merge outstanding chisel-circt PRs first before merging into chisel3?

I'd rather not block this on those PRs. We can move them over here once this lands.

I was instead saying if Jack comes in and wants me to rewrite CIRCTStage.scala, I would instead go do that on sifive/chisel-circt and then rework this PR here to incorporate that. I'm aiming to have a clean break where sifive/chisel-circt ends development and is archived and that end point is where development in chisel3 starts.

ekiwi · 2022-08-04T17:49:24Z

src/main/scala/circt/stage/phases/CIRCT.scala

+            blackbox = true
+            false
+          /* Any emitters should not be passed to firtool. */
+          case _: firrtl.Emitter => false


How do you specify the target if you emitters are ignored? Like how would I distinguish between generating Firrtl or SystemVerilog?

There's a CIRCTTargetAnnotation that will control it. They main issue here is that there isn't a good way of adding multiple RunFirrtlTransformAnnotation(_: firrtl.Emitter)s to CIRCT as it's trying to work towards a fixed target, e.g., FIRRTL Dialect, HW Dialect, SV Dialect, or SV.

It's reasonable that if there was one RunFirrtlTransformAnnotation(_ : firrtl.Emitter) that this could be used to tell CIRCT what to compile to. However, it seemed cleaner to just strip this and force users to use the new annotation.

Thinking about this a bit more... it would probably be better to actually error on this. I can do this in a follow-up.

This should be basically dead code right now as the expectation is that a user is going to use circt.stage.ChiselStage which is not going to use a RunFirrtlTransformAnnotation to tell CIRCT what emitter to use. There's a separate API/Annotation for that, CIRCTTargetAnnotation. A user could see weird behavior if they are doing one of the following:

Generating a RunFirrtlTransformAnnotation(_ : Emitter) inside a ChiselAnnotation.

Building a custom stage out of Chisel3 phases and CIRCT phases.

Possibly other weird things.

mwachs5 · 2022-08-04T17:52:18Z

I agree that if there are tweaks we want to make to chisel-circt we can do that here once this is on.

What is the story for backporting and backwards compatability binary checking for this chunk of code? For now will we just have this on master branch and worry about the binary compatability story once it gets into 3.6?

jackkoenig · 2022-08-04T18:05:26Z

What is the story for backporting and backwards compatability binary checking for this chunk of code? For now will we just have this on master branch and worry about the binary compatability story once it gets into 3.6?

No backporting, this is a Chisel 3.6 addition.

mwachs5 · 2022-08-04T18:11:17Z

Ok, if this is a 3.6 addition and we are stopping any development on the chisel-circt repo, that is a bit tricky for anyone who wants to use 3.5.x and chisel-circt and needs chisel-circt improvements... But I'm Ok with a workflow that makes parallel PRs to chisel-circt and this repo until this is actually released w chisel

seldridge · 2022-08-04T18:31:16Z

that is a bit tricky for anyone who wants to use 3.5.x and chisel-circt and needs chisel-circt improvements

I wouldn't worry about this. chisel-circt is ranked 607574 on Maven. chisel3 is ranked 17385. Also, chisel-circt was always an extremely experimental thing. I don't see a good reason to commit to maintenance of sifive/chisel-circt for a hypothetical user on 3.5.x when the answer is: (1) use SFC for 3.5.x (which is the official policy) or (2) switch to 3.6 (which should be easy if somebody is already relying on chisel-circt which has limitations around supporting experimental Chisel APIs).

…rrtool (#15) - Fix handling of EmitAllModulesAnnotation - Fix stderr handling/erroring out if firtool errors during compile

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

git-subtree-dir: circt git-subtree-mainline: 0b2c211 git-subtree-split: 7dd9f0e

Move the git-subtree added sifive/chisel-circt repository from "circt/" into "src/main/scala/circt/" and "src/test/scala/circtTests/". Update the chisel-circt README, which now lives in the src directory, to not indicate that this is a published dependency anymore. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Use the published Ubuntu 20.04 binary of CIRCT to enable chisel-circt tests in CI. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

seldridge · 2022-08-04T19:33:42Z

I force pushed this after landing a PR that @mwachs5 needed. That closes out any known needed development on sifive/chisel-circt.

seldridge · 2022-08-04T20:22:20Z

Merging after verbal approval from @jackkoenig.

sequencer

Sorry for the late view, but not too late, glad we didn't version 3.6.0 now.
Just some small questions:

Is possible to add as less as possible public APIs to make room for future improvements.
IIRC, the handover behavior will be removed eventually, what's the roadmap of this.

sequencer · 2022-05-07T19:36:03Z

src/main/scala/circt/Implicits.scala

+object Implicits {
+
+  /** Helpers for working with Boolean */
+  implicit class BooleanImplicits(a: Boolean) {
+
+    /** Construct an Option from a Boolean. */
+    def option[A](b: => A): Option[A] =
+      if (a)
+        Some(b)
+      else
+        None
+  }
+
+}


I thought these APIs may not be public?

sequencer · 2022-08-08T14:08:34Z

src/main/scala/circt/Implicits.scala

+object Implicits {
+
+  /** Helpers for working with Boolean */
+  implicit class BooleanImplicits(a: Boolean) {
+
+    /** Construct an Option from a Boolean. */
+    def option[A](b: => A): Option[A] =
+      if (a)
+        Some(b)
+      else
+        None
+  }
+
+}


Sorry for the delay review:
Should we private these to avoid introducing more public APIs?

sequencer · 2022-08-08T14:12:00Z

.github/workflows/test.yml

+      - name: Install CIRCT
+        run: |
+          mkdir usr
+          wget https://github.com/llvm/circt/releases/download/sifive%2F1%2F11%2F0/circt-bin-ubuntu-20.04.tar.gz -O - | tar -zx -C usr/
+          echo "$(pwd)/usr/bin" >> $GITHUB_PATH


Hope we can have a better way to install CIRCT in the system.

sequencer · 2022-08-08T14:13:42Z

src/main/scala/circt/stage/Annotations.scala

+
+}
+
+object CIRCTHandover extends HasShellOptions {


I thought we might eventually get rid of the Handover behavior. Am I right?

sequencer · 2022-08-08T14:15:59Z

src/main/scala/circt/stage/phases/AddDefaults.scala

+import firrtl.AnnotationSeq
+import firrtl.options.Phase
+
+class AddDefaults extends Phase {


should this be private too?

sequencer · 2022-08-08T14:16:35Z

src/main/scala/circt/stage/phases/MaybeSFC.scala

+import firrtl.options.Viewer.view
+import firrtl.stage.{Forms, RunFirrtlTransformAnnotation}
+
+class MaybeSFC extends Phase {


seldridge and others added 27 commits January 13, 2021 21:02

Initial Commit

c136d11

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add documentation

2460754

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add CI

1046d8c

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Merge pull request #1 from sifive/dev/seldridge/ci

2f3c5cb

Add CI

Merge pull request #2 from sifive/dev/seldridge/configurable-handover

dfaed69

Add --handover option to lower partially using SFC

Add publishing to sonatype

042a276

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Merge pull request #3 from sifive/dev/seldridge/sonatype-publish

92041a5

Add publishing to sonatype

Add description, SiFive homepage to build.sbt

80fa20c

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Update README.md

13b9a9e

- Add Maven release, Sonatype snapshots, and Javadoc.io badges - Improve setup instructions Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Bump project dependencies

493988e

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add "--target verilog" option for "*.v" output

17c9d39

Add an option to output using a "*.v" suffix. This is still using the CIRCT SystemVerilog back end, though. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Fix bug where -enable-lower-types was always used

26bc35c

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Switch to -lower-types

0c21356

Change command line option passed to "firtool" due to upstream change. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Switch from FlatSpec to FunSpec

cf2993b

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Change "RTL" to "HW"

caeb131

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Merge pull request #5 from sifive/dev/seldridge/rtl-to-hw

ef09fa0

Change "RTL" to "HW"

Update CIRCT Phase to use newer CIRCT CLI (#8)

bdc5ae4

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add Mill build and Bump to Chisel 3.5 (#7)

f1bdfe4

* add mill build system * bump to Chisel 3.5.0 * use cross build to select Scala version. * add gitignore * Add/use Compiler Plugin

Add Scala 2.13, enable 2.13 CI

b333604

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Merge pull request #10 from sifive/dev/seldridge/scala-2.13

3e95ad7

Add Scala 2.13, enable 2.13 CI

Change default handover to CHIRRTL

54f881d

Change the default handover from optimized low FIRRTL to CHIRRTL. This is done because CIRCT is an essentially complete FIRRTL compiler now. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Merge pull request #6 from sifive/dev/seldridge/issue-4

8ff8aab

Add Annotation support and conversion to firtool CLI options.

Update README badges to 2.13, README updates, NFC

72bd6af

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

mwachs5 and others added 2 commits July 6, 2022 20:30

Add scalafmt Formatting and CI Checks [NFC] (#16)

5554b38

* Add scalafmt files from chisel3 * Enable scalafmt in SBT * Scalafmt all the files * Add scalafmt check to CI

Add support for new preserve-aggregate options (#17)

e1f8778

Add support for the new, finer-grained aggregate preservation options that were added to CIRCT/firtool. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

seldridge force-pushed the dev/seldridge/chisel-circt branch from 6d87c27 to fb844be Compare August 4, 2022 05:15

seldridge marked this pull request as ready for review August 4, 2022 05:36

seldridge added the Merge with merge commit Please merge with a merge commit, do not squash and merg label Aug 4, 2022

seldridge requested a review from jackkoenig August 4, 2022 06:14

ekiwi reviewed Aug 4, 2022

View reviewed changes

mwachs5 and others added 5 commits August 4, 2022 15:24

Handle EmitAllModulesAnnotation and Better grabbing of stderr from fi…

b2b740f

…rrtool (#15) - Fix handling of EmitAllModulesAnnotation - Fix stderr handling/erroring out if firtool errors during compile

Fix scala-fmt

7dd9f0e

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

Add 'circt/' from commit '7dd9f0e9e05b0b2cf1ff5c8cab8b0c490836d021'

73551f0

git-subtree-dir: circt git-subtree-mainline: 0b2c211 git-subtree-split: 7dd9f0e

Add chisel-circt to CI

abab0dd

Use the published Ubuntu 20.04 binary of CIRCT to enable chisel-circt tests in CI. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>

seldridge force-pushed the dev/seldridge/chisel-circt branch from fb844be to abab0dd Compare August 4, 2022 19:32

mwachs5 approved these changes Aug 4, 2022

View reviewed changes

seldridge merged commit cc55936 into master Aug 4, 2022

seldridge deleted the dev/seldridge/chisel-circt branch August 4, 2022 20:21

sequencer reviewed Aug 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git-subtree add sifive/chisel-circt #2477

git-subtree add sifive/chisel-circt #2477

seldridge commented Apr 6, 2022 •

edited

Loading

ekiwi commented Apr 6, 2022

seldridge commented Apr 6, 2022

ekiwi commented Apr 6, 2022

mwachs5 commented Aug 3, 2022

CLAassistant commented Aug 4, 2022 •

edited

Loading

seldridge commented Aug 4, 2022

seldridge commented Aug 4, 2022

mwachs5 commented Aug 4, 2022 •

edited

Loading

seldridge commented Aug 4, 2022

ekiwi Aug 4, 2022

seldridge Aug 4, 2022

seldridge Aug 4, 2022

mwachs5 commented Aug 4, 2022

jackkoenig commented Aug 4, 2022

mwachs5 commented Aug 4, 2022 •

edited

Loading

seldridge commented Aug 4, 2022 •

edited

Loading

seldridge commented Aug 4, 2022

seldridge commented Aug 4, 2022

sequencer left a comment

sequencer May 7, 2022

sequencer Aug 8, 2022

sequencer Aug 8, 2022

sequencer Aug 8, 2022

sequencer Aug 8, 2022

sequencer Aug 8, 2022

git-subtree add sifive/chisel-circt #2477

git-subtree add sifive/chisel-circt #2477

Conversation

seldridge commented Apr 6, 2022 • edited Loading

ekiwi commented Apr 6, 2022

seldridge commented Apr 6, 2022

ekiwi commented Apr 6, 2022

mwachs5 commented Aug 3, 2022

CLAassistant commented Aug 4, 2022 • edited Loading

seldridge commented Aug 4, 2022

seldridge commented Aug 4, 2022

mwachs5 commented Aug 4, 2022 • edited Loading

seldridge commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwachs5 commented Aug 4, 2022

jackkoenig commented Aug 4, 2022

mwachs5 commented Aug 4, 2022 • edited Loading

seldridge commented Aug 4, 2022 • edited Loading

seldridge commented Aug 4, 2022

seldridge commented Aug 4, 2022

sequencer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge commented Apr 6, 2022 •

edited

Loading

CLAassistant commented Aug 4, 2022 •

edited

Loading

mwachs5 commented Aug 4, 2022 •

edited

Loading

mwachs5 commented Aug 4, 2022 •

edited

Loading

seldridge commented Aug 4, 2022 •

edited

Loading