-
Notifications
You must be signed in to change notification settings - Fork 194
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 UHDM frontend support for Yosys using Surelog #202
Conversation
Some context - this allows synthesis of SystemVerilog designs in Yosys using Surelog parser. Surelog passes information about the design using UHDM. Here https://github.com/alainmarcel/uhdm-integration/pull/97/checks?check_run_id=1536854237 you can find an example Ibex build. The design builds fine and can be P&Rd with e.g. Vivado giving working bitstream. |
Thanks! I'll give this a try in the new year with Ibex. |
I've rebased this on top of master |
edalize/surelog.py
Outdated
if api_ver == 0: | ||
return {'description' : "Surelog", | ||
'lists' : [ | ||
{'name' : 'library_files', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs som explanation. I get the feeling it is verilog "libraries", i.e. a directory with files that have the same filename as the modules inside, or is it something else? If it is verilog libraries I wonder if this is really the method we prefer. It should be possible to specify these as ordinary source files instead.
Generally when files are added as tool_options we must be very careful about thinking how the user will pass them into Edalize. In most cases it's better to add a new file type if we must
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surelog requires access to definition of all modules to match the usage with the model specification (without this, Surelog will be missing parameter information for those modules).
This parameter allows to pass such files as a libraries (more information available in: https://github.com/alainmarcel/Surelog/issues/893).
I tried to pass them as a logical_name
file_type , but when using edalize
with fusesoc
, files can only be specified relative to the .core
location (and this files are usually stored outside of that location). If there is better way to pass this files that will work with fusesoc
I will change this.
Example of such library file: https://github.com/antmicro/yosys/blob/uhdm-yosys/techlibs/xilinx/cells_xtra_surelog.v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Olof, the library_files directive here looks wrong and probably needs to go -- I'm just not quite sure yet how. To get us closer to that answer: Are these libraries needed when a user actually uses a Xilinx primitive in their code, or are they always needed when the synthesis tool decides to infer a primitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are only needed, when a user uses a Xilinx primitive in their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially any vendor primitives (this is not limited to Xilinx)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, a follow-up then: How are those libraries distributed? Are they part of Surelog, or of (the modified version of) Yosys? Or is the user expected to provide those files themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are distributed as a part of yosys
or symbiflow_toolchain
(in case of building using symbiflow_toolchain
instead of vivado
).
edalize/surelog.py
Outdated
(src_files, incdirs) = self._get_fileset_files() | ||
file_list = [] | ||
for f in src_files: | ||
if f.file_type in ['verilogSource']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will ignore e.g. verilogSource-95 types. Use startswith instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
edalize/surelog.py
Outdated
for key, value in self.vlogparam.items(): | ||
verilog_params_command = verilog_params_command + " -P" + key + '=' + value | ||
|
||
include_files = self.tool_options.get('include_files', []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do I find these include_files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was unused leftover, I removed this.
I'm using incdirs
that is returned by self._get_fileset_files()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first set of comments form my side. All of the current Yosys backend work is quite FPGA-specific where it doesn't need (or should) be; I need to have a closer look at that first, because it seems that this PR pushes into that direction even more.
I'm also not really happy with the fact that we now effectively have a tree of options (vivado -> vivado/yosys -> surelog/plain-yosys) all slightly intermixed. Maybe we need to just start calling different tool "mixes" and patched versions differently, e.g. "vivado-yosys" or "vivado-synplify" or "yosys-surelog", as long as the tool (vendor) doesn't integrate those options into a single product.
edalize/surelog.py
Outdated
if api_ver == 0: | ||
return {'description' : "Surelog", | ||
'lists' : [ | ||
{'name' : 'library_files', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Olof, the library_files directive here looks wrong and probably needs to go -- I'm just not quite sure yet how. To get us closer to that answer: Are these libraries needed when a user actually uses a Xilinx primitive in their code, or are they always needed when the synthesis tool decides to infer a primitive?
@@ -0,0 +1,27 @@ | |||
#Auto generated by Edalize | |||
SHELL = bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not, removed
edalize/surelog.py
Outdated
template_vars = { | ||
'top' : self.toplevel, | ||
'name' : self.name, | ||
'sources' : ' '.join(file_list), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation, surelog differentiates between Verilog and SystemVerilog parsing. Currently, the call always forces Surelog to operate in SystemVerilog mode and then passes files as "Verilog". If Surelog makes that difference, we should honor the file type passed in FuseSoC and pass on files with verilogSource and systemVerilogSource differently. (If Surelog treats those files the same internally, maybe its command line options should be cleaned up.)
-sverilog Forces all files to be parsed as SystemVerilog files
-sv <file> Forces the following file to be parsed as SystemVerilog file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we don't force SystemVerilog mode anymore. By default we let Surelog handle Verilog and SV internally.
@imphil - I recently tried to again map out the how the different parts of the ecosystem connect to each other. You can find it at https://docs.google.com/drawings/d/1ocoKZFu2gJodAgBku4yTePjdXSrl2tBg95czmqivvY8/edit I really think we should move edalize towards an approach were it understands things like "language frontend" plugins and also the different stages like "synthesis" verse "place and route". It is perfectly reasonable to want to use
Maybe the Yosys synthesis tool could provide a list of "language frontends" and each "language frontend" can register for which file types it supports? Still unsure how you handle things like Yosys plugins for optimization like |
library_files directive was removed. We define "arch" instead and use "/cells_bb.v" in yosys-config call in Makefile. |
Add EDALIZE_LAUNCHER to verilator template
Hi, Just wanted to say that I'm intending to review this PR next but just haven't found the time yet. Soon come! |
FYI, this will take a bit more time. The reason now is that I realized that this addition will be a lot more manageable if we first can do some refactoring that is needed anyway for Edalize (Slight Return). I filed a tracking issue to make this a bit more visible #243 |
@kamilrakoczy @kgugala @kamilrakoczy @mithro It would be great if I could get some help from you. As mentioned in #243 I need to do some refactoring first before I can merge this one. What I need help with is to a) check that the current changes on the https://github.com/olofk/edalize/tree/esr1 branch didn't break anything (I think fpga-tool-perf is the main user here) and b) get help converting the rest of the backends in the way that I have done with the three first ones. So if anyone time to help that would help me unblock this one faster |
@olofk I must have missed the notification from this one. I'll find somebody to push this further |
Thanks @kgugala! I pushed a bunch of more patches now to heopfully make it clearer what I'm trying to achieve |
…ontend_optins in yosys to reflect that
@olofk testing the whole perf tool on the refactored branch will not be easy as the perf tool uses a fork which is still quite far from mainline for now and it's not very simple to rebase it all and test it quickly. We did however run some limited tests of it, only for icestorm for now, as this part required less effort, we've had to add one fix to your branch to make it pass, here is the patch: antmicro@8edd8c8 The next step for us will be to rebase the work from this PR onto your refactored branch |
Thanks @tgorochowik . It's exactly these kind of issues I was worried about. Great that you caught it. How would you feel if I merged my esr1 branch to master with that fix applied? I'm a bit worried that it hasn't received much testing, but OTOH it might be quicker to work out issues if it's in master, and perhaps easier for you to add your patches on top of master instead of a branch. I'm fully aware that the rebase will be painful but a major goal with the refactoring is to make the integration of new tools, features and flows easier so hopefully it will be smoother from now on. Also, I'm done with the big refactoring now. There are probably a couple of cleanups left but other than that all Yosys-using backends are converted. And with that I think we can start landing the surelog and sv2v support piece by piece. I have some ideas for the order of things
After that we can start the integration but there are some details to work out
What do you think? |
We're going to have to rebase everything anyway so I'd say the sooner we do this the better.
We'll start by doing it the way you described and try to come up with some ideas on how to address the details that need to be worked out along the way. This way it will be easier for us to fully understand the current state and propose some ways to continue. Is that okay?
We don't use slang for anything that might require integration with edalize at the moment, but it certainly can be integrated at a later stage (CC @MikePopoloski) |
Right now slang only has two output formats -- JSON and a C++ AST library. I would not recommend JSON as an interchange format, it will be far too bulky and slow for larger projects. The C++ library is ideal from a compilation speed perspective but obviously it requires custom work by each consumer. I'm not against adding UHDM output; I think it would probably be pretty simple (and I believe it uses Cap'n Proto for the database which is a good choice), though when I looked in the past all the docs seemed to be geared towards consumers and I didn't spend much time digging into what was needed on the producer side. This is an area that would be relatively straightforward for someone to contribute to slang if they wanted. |
Great. I agree. It's a big patcheset, but we just had a release also, so this is probably the safest time to merge potentially broken stuff. It's in master now.
That sounds great to me! |
Just to provide an update here: The current state of Edalize (with links to current code) is tracked here: antmicro/yosys#443 The state of the plugin itself is tracked here: antmicro/yosys#442 For the time being we're going to keep it separated, when the plugin is ready and usable we will contribute the edalize changes to this repo (either in this PR or separately in a new one). |
Thanks for the update @tgorochowik. FYI, I would still be happy to pull sv2v support first if you want to keep the whole patchset smaller. |
A couple of weeks ago I landed the first patches of the new flow API which supports using surelog as a frontend. Does this do the job so we can close this PR? |
This PR adds UHDM frontend for yosys.