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

Implement Verilog output stream PanamaCIRCTConverter #3520

Merged
merged 10 commits into from
Oct 14, 2023

Conversation

SpriteOvO
Copy link
Member

@SpriteOvO SpriteOvO commented Sep 3, 2023

Based on llvm/circt#6094 and llvm/circt#6036.

We are trying to reimplement CIRCT firtool in Chisel, with it, we can select and manipulate passes to export to targets with the new CIRCTConverter in #3466.

This PR implements Verilog output stream PanamaCIRCTConverter

This PR is at a very early stage, opening the PR for discussion.


Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • 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 add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Feature (or new API)
  • API modification
  • API deprecation
  • Backend code generation
  • Performance improvement
  • Bugfix
  • Documentation or website-related
  • Dependency update
  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).
  • Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.5.x, 3.6.x, or 5.x depending on impact, API modification or big change: 6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@SpriteOvO SpriteOvO mentioned this pull request Sep 3, 2023
24 tasks
@SpriteOvO SpriteOvO changed the title Reimplement CIRCT firtool inside Chisel Implement Verilog output stream PanamaCIRCTConverter Sep 10, 2023
@sequencer
Copy link
Member

We notice width truncation was broken when we try to elaborate the T1 testbench, thanks @seldridge, pointed me the link saying parser is adding additional ops for padding.
Since we are directly issuing MLIR ops in this flow, there won't be any fixing from parser. So it will be slightly difference between firtool and firtool-lib. In fact firrtool-lib will be stricter(which I prefer)

Since it is stricter, I will personally force T1 and rocket-chip to avoid width padding and trunction in connect, and fix them.

But this is an issue we should try to fix in the future, here are some possible solutions:

  • remove this logic in parser and add a special post-parse pass, so that we can share this logic between firtool and firtool-lib.
  • add these ops in the chisel builder or converter to work around
  • make the truncation and padding as warning, and forbid them in the future.(user should explicit padding and truncating)

Since we didn't release panama converter, and won't release it to normal user in a near future, when that day comes, we may already fix this issue. So it won't block the merging of this PR.

For any project which tries to use this feature. please fix the this issue in your user code.

@SpriteOvO SpriteOvO force-pushed the firtool-in-chisel branch 4 times, most recently from 987d43e to 07c139a Compare September 24, 2023 12:59
binder/src/test/scala/BinderSpec.scala Outdated Show resolved Hide resolved
binder/src/test/scala/BinderSpec.scala Outdated Show resolved Hide resolved
binder/src/main/scala/PanamaCIRCTConverter.scala Outdated Show resolved Hide resolved
@SpriteOvO SpriteOvO force-pushed the firtool-in-chisel branch 4 times, most recently from 7c32da6 to f65adad Compare September 26, 2023 18:03
@sequencer sequencer mentioned this pull request Oct 3, 2023
16 tasks
@sequencer sequencer force-pushed the firtool-in-chisel branch 3 times, most recently from f4c6c35 to 596bdc9 Compare October 14, 2023 13:17
refer to:
- openjdk/jdk#13079
- llvm-project/circt dadf87b742f547840f6866beb50bdf46a76d3fed
@sequencer sequencer added Dependency Update Updates a dependency, will be included in release notes Internal Internal change, does not affect users, will be included in release notes and removed Dependency Update Updates a dependency, will be included in release notes labels Oct 14, 2023
@sequencer sequencer marked this pull request as ready for review October 14, 2023 14:54
Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

Change in this PR doesn't affect the review flow, and possibly not be included in Chisel 6, so I mark this as internal.

After this PR, the chisel-circt-binding project is able to emit Verilog by calling CIRCT C-API, thanks hard work by @SpriteOvO

The next milestone should possibly be pass plugin, e.g:

  • inspect Property via Scala.

This feature is in a very early stage! please don't use in production flow for now!

@sequencer sequencer added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Oct 14, 2023
@mergify mergify bot merged commit 73a50d0 into chipsalliance:main Oct 14, 2023
17 checks passed
adkian-sifive pushed a commit that referenced this pull request Oct 16, 2023
* Implement Verilog output stream `PanamaCIRCTConverter`

* Truncate or pad when connecting ports with different widths

* Use `scala.math.BigInt.bitLength` instead of our own `bitLength`

* Use correct module name for instances

* Binder introduces Firtool options

* fix binder for JDK21 and firtool 1.67.0

refer to:
- openjdk/jdk#13079
- llvm-project/circt dadf87b742f547840f6866beb50bdf46a76d3fed

* fix for review

* reformat

* bump GitHub CI to JDK 21 and mill 0.11.5

* remove 2.13.0 to 2.13.10 for JDK21 in build.sc

ref to: scala/bug#12783

---------

Co-authored-by: Jiuyang Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Internal change, does not affect users, will be included in release notes 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.

2 participants