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

New Hammer #1324

Merged
merged 30 commits into from
Feb 15, 2023
Merged

New Hammer #1324

merged 30 commits into from
Feb 15, 2023

Conversation

harrisonliew
Copy link
Contributor

@harrisonliew harrisonliew commented Feb 3, 2023

This PR switches the VLSI flow over to new Hammer, which is now published on PyPI: https://pypi.org/project/hammer-vlsi/.
This main package is installed via conda, and plugins packages are installed with pip -e using the init-vlsi.sh script.

Several bugs/problems are caused by the migration to CIRCT, and should be investigated:

  • SystemVerilog packed arrays are being emitted, despite adding the disallowPackedArrays lowering flag. This means Yosys is unable to synthesize designs, and there is no roadmap/effort there to support multidimensional arrays. Thus, the Sky130 + OpenROAD tutorial therefore does not work anymore.
    Fixed by [HWLegalizeModules] Legalize aggregate constant llvm/circt#4626
  • Async resets are being emitted in a way that synthesis tools like Genus do not like. Specifically, we get something of the form:
     wire reset = ~_some_reset;
     always @(posedge clock or posedge reset)
     	if (~_some_reset)
     		...
    
    This is not allowed because the reset in the if(... should be the one in the sensitivity list.
    Attempts to use the exprInEventControl lowering flag have not solved this.
    The workaround is to manually change if(~_some_reset) to if(reset). If we are sticking with this workaround, I will update the documentation.
    Fixed by [ExportVerilog][PrettifyVerilog] Fix exprInEventControl llvm/circt#4625
  • Line 64 of vlsi/Makefile is an ugly hack to separate out the harness blackboxes that cannot be input to synthesis tools due to the presence of DPI calls. It would be best to investigate ways to restore the top/harness blackbox separation in CIRCT.
    Fixed by @joey0320

Finally, these dependencies need to be included:

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

vlsi/Makefile Outdated Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor

For future reference: To replicate the CIRCT issues you can try building a TinyRocketConfig

@joonho3020
Copy link
Contributor

For the packed array thing, we can go over this issue for now by setting ENABLE_CUSTOM_FIRRTL_PASS=1

@harrisonliew
Copy link
Contributor Author

Unfortunately, even with using ENABLE_CUSTOM_FIRRTL_PASS=1, there are still remaining SystemVerilog constructs that Yosys doesn't recognize, such as:
some_wire <= 10'(some_wire - 10'h1);

It would be nice to have Hammer's Yosys plugin call this: https://github.com/zachjs/sv2v, but the pre-built images require quite new glibc...

@abejgonzalez
Copy link
Contributor

I added sv2v to our conda repo: https://github.com/ucb-bar/sv2v-feedstock. I tested that running it worked with TinyRocketConfig sv2v *.sv for all files in the gen-collateral area. You should be able to install it with conda install -c ucb-bar sv2v (I would add it to the Hammer requirements when it's build by conda though).

@uenoku
Copy link

uenoku commented Feb 6, 2023

here are still remaining SystemVerilog constructs that Yosys doesn't recognize, such as:
10'(some_wire - 10'h1)

Hi, these static casts are added when explicitBitcast is given so can you try removing the option from

--lowering-options=emittedLineLength=2048,noAlwaysComb,disallowLocalVariables,explicitBitcast,verifLabels,locationInfoStyle=wrapInAtSquareBracket \
?

@harrisonliew harrisonliew marked this pull request as ready for review February 10, 2023 21:12

$(VLSI_RTL): $(RTL_DEPS)
ifneq ($(CUSTOM_VLOG), )
> $(VLSI_RTL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like something was deleted here

Copy link
Contributor Author

@harrisonliew harrisonliew Feb 15, 2023

Choose a reason for hiding this comment

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

No. This writes a blank file. Equivalent to echo -n > $(VLSI_RTL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, turns out this breaks in csh-based shells (it should be : > $(VLSI_RTL)) but our scripts require bash anyway. Good to know for the future.

Copy link
Member

@sagark sagark left a comment

Choose a reason for hiding this comment

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

LGTM. this works for me as of b937a27 (unfortunately don't have time to bump further right now).

I also pushed one small tweak to the Makefile -- I couldn't suggest change because that part of the Makefile was otherwise untouched. Feel free to revert if there's some problem with the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:changed dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants