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

[syn] Add initial Yosys synthesis script with example lib #342

Merged
merged 1 commit into from
Nov 29, 2019

Conversation

NilsGraf
Copy link
Contributor

@NilsGraf NilsGraf commented Sep 20, 2019

This PR includes the following:

  • add script syn_yosys.sh, which runs sv2v and yosys for ibex_core
  • add example std. cell lib cmos_cells.lib (copied from yosys repo)
  • add dummy prim_clock_gating.v module
  • add initial yosys synthesis script syn.ys

@NilsGraf
Copy link
Contributor Author

Update: sv2v tool has been fixed now, so I removed the work-around.

Copy link
Contributor

@msfschaffner msfschaffner left a comment

Choose a reason for hiding this comment

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

Looks ok to me, but I will let others (with more Yosis experience) approve.

Copy link
Collaborator

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

Only been playing with yosys myself for a week or so so I'm not expert but I do have a few comments.

As far as I can tell the native yosys scripting language is very limited in that it doesn't allow any variables or conditional execution. It does however allow TCL scripting. I wonder if it's better to take an approach of just calling the TCL script from syn.ys (if you can't directly specify a TCL script on the yosys command line) so we can do things like specify standard cell library, say we want an interactive run (one that dumps you into the yosys shell at the end or you can specify one of several points to be brought into the shell) choose if we want flatten or not etc.

There's also the question of what do we want to achieve with this flow? Are we better off just grabbing the OpenROAD flow for example?

# yosys synthesis script

read -sv prim_clock_gating.v syn_out/*.v
synth -top ibex_core
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want -flatten here? I've prefered a flattened hierarchy for probing around the design. When you leave the modules it this can be confusing (I'm not sure tracing logic cones and such goes past module boundaries or I remember having trouble with this, the longest topological path command, ltp, also doesn't seem to deal with it well declaring you have loops in your design because there's some cell that takes input from a module as well as outputs to it).

I suspect flattening gives better synthesis results also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can add -flatten. I'm just using the example synthesis script given in the Yosys documentation.

Alternatively, do you want to LGTM this PR and then you can create a follow-on PR with all your improvements?

write_verilog ibex_core_premap.v

# mapping to cmos_cells.lib
dfflibmap -liberty cmos_cells.lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

The cmos_cells.lib whilst fine for demonstration purposes or exploring how yosys works is pretty useless for actual synthesis. Should be fine for this initial PR but we do need to source a better library that we can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely. At a minimum, this library should include a MUX2 and also an ICG.

Do you know of a good open-source std. cell library? Yosys documentation has a link to some but not sure which one is good.


# yosys synthesis script

read -sv prim_clock_gating.v syn_out/*.v
Copy link
Collaborator

Choose a reason for hiding this comment

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

a 'read_liberty -lib cmos_cells.lib' could be useful here. This means that when using the show command and otherwise exploring the design yosys understands what the cells are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I can add it (or alternatively, if you just want to LGTM this PR and then create another one with your improvements?)

rtl/ibex_core.sv Outdated Show resolved Hide resolved
@sjgitty
Copy link
Contributor

sjgitty commented Sep 26, 2019

There's also the question of what do we want to achieve with this flow? Are we better off just grabbing the OpenROAD flow for example?

This is a great point to socialize. At a minimum it would be nice to see this work
act as a CI check on synthesizability. At a maximum of course it would be for
someone to be able to target to a real silicon design. A nice in between goal
is to use it to create a regular report of QoR over time to ensure that
fixes and improvements don't affect area / speed etc.

@GregAC
Copy link
Collaborator

GregAC commented Sep 26, 2019

I have been experimenting with OpenSTA along with the Nangate library from the OpenROAD repository Doesn't take much more work on top of this to get what appears to be a plausible timing report which could form the basis of a CI job. Only a couple of minutes run time to produce it on my laptop.

As for area yosys can give you cell area with the stat command when you pass the library with a -liberty switch.

@NilsGraf
Copy link
Contributor Author

NilsGraf commented Sep 26, 2019

There's also the question of what do we want to achieve with this flow? Are we better off just grabbing the OpenROAD flow for example?

I would love to use OpenROAD. But as far as I understand, OpenROAD uses Verilog, not SystemVerilog as input. They use a Cadence synthesis tool to synthesize the Ibex SystemVerilog into a Verilog netlist, see here.

This PR here uses sv2v tool to convert the SystemVerilog into Verilog. Once this PR is committed, I will contact the OpenROAD folks and ask them to include sv2v into the OpenROAD flow.

@NilsGraf
Copy link
Contributor Author

have been experimenting with OpenSTA along with the Nangate library from the OpenROAD repository Doesn't take much more work on top of this to get what appears to be a plausible timing report which could form the basis of a CI job. Only a couple of minutes run time to produce it on my laptop.

As for area yosys can give you cell area with the stat command when you pass the library with a -liberty switch.

Awesome. It would be great to add that in a subsequent PR.

@GregAC
Copy link
Collaborator

GregAC commented Sep 27, 2019

I would love to use OpenROAD. But as far as I understand, OpenROAD uses Verilog, not SystemVerilog as input.

I confess I hadn't looked into it in much detail but given it used yosys as the frontend tool I assumed anything we could get pushed through our own yosys based flow would go through OpenROAD. Maybe other parts of the flow want the original RTL rather than a synthesised version? Something worth experimenting with.

Copy link
Collaborator

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

Ok LGTM. Clearly an area of much complexity and possibilities but this is a good start. I'll get a PR done with some of my suggestions. Thanks for the PR @NilsGraf

@@ -0,0 +1,61 @@
// Copyright lowRISC contributors.
Copy link
Member

Choose a reason for hiding this comment

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

Please replace this license header with the Yosys license:

yosys -- Yosys Open SYnthesis Suite

Copyright (C) 2012 - 2019  Clifford Wolf <clifford@clifford.at>

Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above
copyright notice and this permission notice appear in all copies.

THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! I added the copyright.

FYI, the original file in yosys repo doesn't have any copyright info.

Copy link
Collaborator

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

Given you'll need to update for Alex's request can you make the 'Update ibex_core.sv' commit more specific? 'Fixed missing parameter in ibex_core' for example.

@NilsGraf
Copy link
Contributor Author

Given you'll need to update for Alex's request can you make the 'Update ibex_core.sv' commit more specific? 'Fixed missing parameter in ibex_core' for example.

Sure, I removed this change, I believe you suggested to create a separate PR for this, which I will do shortly. I'm now also updating the description of this PR to remove the following item
- also fixed a small issue in ibex_core.sv to pass parameter PMPEnable to the CSRs

@NilsGraf
Copy link
Contributor Author

NilsGraf commented Sep 27, 2019

FYI, I created PR #356 for the missing parameter assignment.

@asb
Copy link
Member

asb commented Sep 30, 2019

Hi Nils, could you please use git interactive rebase to merge the three commits in this PR?

syn/syn_yosys.sh Outdated Show resolved Hide resolved
@towoe towoe mentioned this pull request Oct 8, 2019
5 tasks
@GregAC
Copy link
Collaborator

GregAC commented Oct 8, 2019

Just a note that I have built a flow on top of this PR that gives you timing reports with OpenSTA and makes things more configurable using environmental variables and TCL to drive the tools. Still needs some cleaning up but I'll make a PR of it soon.

@NilsGraf
Copy link
Contributor Author

NilsGraf commented Oct 8, 2019

Awesome! How can I commit this PR so you can go ahead?

@imphil
Copy link
Contributor

imphil commented Oct 8, 2019

I'm sorry for the delay here, it's on my TODO list to review this and I hope to get to it later this week.

@NilsGraf
Copy link
Contributor Author

NilsGraf commented Oct 8, 2019

No worries. This PR already has 1 LGTM and approval. But it also has a block from Alex asking for changes. Hi Alex, is the interactive rebase still needed? Otherwise, I was just going to hit "squash and merge".

This PR includes the following:
- add script syn_yosys.sh, which runs sv2v and yosys for ibex_core
- add example std. cell lib cmos_cells.lib (copied from yosys repo)
- add dummy prim_clock_gating.v module
- add initial yosys synthesis script syn.ys
@imphil
Copy link
Contributor

imphil commented Nov 29, 2019

Rebased and filed follow-ups for the outstanding issues:

@GregAC please file follow-ups for the remaining issues you identified.

@imphil imphil merged commit 260ed5a into lowRISC:master Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants