olofk / fusesoc

Package manager and build abstraction tool for FPGA/ASIC development
BSD 2-Clause "Simplified" License
1.16k stars 242 forks source link

Support lint tools #158

Closed imphil closed 6 years ago

imphil commented 7 years ago

Currently fusesoc is pretty much centered around building a synthesis or a simulation. I've recently come across the need to also support linting with different tools, such as Verilator (using --lint-only) or commercial tools like SpyGlass. All these tools have in common that they also need the file/include list as it's generated by fusesoc.

Possible design options (or a mix of some of them):

  1. Add a new "lint" class of tools, similar to "build" and "sim"
  2. Remove the special-casing of build and sim tools, and make fusesoc more of a "tool runner". sim and build can remain "categories" of tools, however.

Orthogonal to that, the question of a "build sequence" comes up. Should one call to fusesoc do one step in a build process (such as "run synthesis" or "build simulation"), or could it also be a sequence of steps (such as "run lint first, then synthesize and then flash to FPGA").

olofk commented 7 years ago

Yes, I've also been thinking how to add support for linters. It has been on my wishlist for a long time. Thanks for bringing it up

  1. One problem arises around which files to lint. Each tool can have tool-specific files. A way to solve that could be to instead add a --lint flag to the backend, such as there already are for --setup or --build-only

  2. Yes. They are sharing more and more code, and some tools are actually a bit hard to categorize. I would need a good reason to make this change however. Do you have any example?

There was a patch (https://github.com/olofk/fusesoc/pull/140) some time ago to do something similar. My reason at the time not to accept it was that I thought this could just as well be handled by a shell script. That would also make it easier to have conditions, such as only run synthesis if lint succeeds. I think you can already call most of the steps in the build process separately, even if it's not very clear how. --setup only runs the configure stage, --build-only runs configure+build, --keep will only run the simulation without rebuilding. I guess this could need some cleanup :)

imphil commented 7 years ago

WIP of a SpyGlass lint implementation is available in https://github.com/imphil/fusesoc/tree/spyglass (Also featuring Jinja templating; I'll clean that up and split it out for #166).

Status:

  1. It's a build tool, so it requires a separate *.core file with backend = spyglass. We need to find a solution here, in line with what was discussed above.
  2. It only writes the project file, doesn't perform the lint run yet
  3. It doesn't allow to select a lint goal yet, I need to extend the [spyglass] section for that.

What's really blocking this now is the question how to integrate lint tools in general. Personally, I'd go with

olofk commented 7 years ago

Thanks for doing this. I don't have time to look at the actual spyglass implementation right now, but I'll answer your general points

Yes, I think you're right. It's time to give up the distinction between tools. There is already very little code that differs between the build and sim backends. I think the timing is right too, as I'm hoping to finish up CAPI2 and this will have an impact on the decisions there. We should probably bike-shed the actual implementation details a bit, but I think a fusesoc run command would be good, with --setup, --build and --launch to run any of the three stages. (Note. No flags would run them all. launch corresponds to pgm for "build" backends). I think in that case that we can't auto-detect which tool to use (which is probably ok actually) and have to pass it on the command-line, like fusesoc <fusesoc options> run <general run-options> vivado <vivado_options> supersoc <soc_options>. Actually... that's quite good as it would give us a natural place to add tool-specific extra-options. I like that. We need to think a bit about how to handle the --target parameter though (what used to be --testbench for simulation flows, but will support different synthesis/P&R targets as well with CAPI2), but it should be doable

A small note regarding caching results. FuseSoC used to do this by default up until FuseSoC 1.2, which meant that users had to run with --force almost every time to avoid reusing old files. I planned to do some smart caching here to only update if the sources changed, but never got around to do that, so I added the --keep argument instead to let users decide

Agree that handling more advanced pipelines is better with shell scripts

olofk commented 7 years ago

And we can also give some adhoc names to tool flows, at least to start with. Like verilator for simulations and lint-verilator to use it only for linting

olofk commented 6 years ago

The framework for this is in place now. Just ran linting with Verilator on a CAPI2 version of mor1kx-generic. Just adding tests for some bugs I found in the process and I will check in. After that we can look at merging the spyglass backend

imphil commented 6 years ago

Let me know when you have the verilator lint backend ready to be tested. I'll then give it a try and see if I can get the spyglass backend ported (and extended) as well. We currently have a colleague working on it to support VHDL for a mixed-language design, I'll try to get these changes integrated as well.

olofk commented 6 years ago

Done. Check out the capi2 branch of fusesoc-cores and run fusesoc run --target=lint mor1kx-generic

imphil commented 6 years ago

What do I need to test this on OpTiMSoC? A couple questions:

olofk commented 6 years ago

The top-level core must be capi2. I think we reached the end of the road with capi1 when it comes to new features. I spent a lot of effort trying to cover the cases with capi2 cores depending on capi1 cores, so hopefully this shouldn't be a problem, but I expect to find some corner case bugs. There isn't yet any tool for migrating cores. It's something I have considered, but not prioritized yet. Also, capi2 is still experimental so things might change in places. I will file a new issue for having such a tool created. My recommendation right now is to do the following. Manually migrate the top-level to capi2 (happy to help). Bump the revision of the new core file. Old FuseSoC will ignore the capi2 file, while the new one will use it as it has a higher revision (you can always do an exact match if you want to use the old core with newer FuseSoC, e.g. fusesoc build =optimsoc-0).

toplevel is an array because verilog (not sure about vhdl) supports multiple top-levels. This is for example used extensively by Xilinx models which needs glbl.v to be loaded as a parallel toplevel. I have very much considered to allow regular strings here too as that is the most common case, but I want to wait with special cases a bit until other bugs are worked out.

imphil commented 6 years ago

It seems to mostly work, but I've found CAPI2 to be too alpha to test larger designs:

So most likely this issue can be closed, but I really don't know if what works today is enough to run lint in a larger OpTiMSoC design.

olofk commented 6 years ago

It seems to mostly work, but I've found CAPI2 to be too alpha to test larger designs:

Thanks. This is great feedback and I can see now that it's even less ready than I thought.

No Vivado support?

Correct. I need to add support for all tools

No real documentation or examples I could find

No documentation whatsoever. There are some examples in the capi2 branch of fusesoc-cores. The de0_nano core is pretty well commented, but the comments are mostly aimed for me during development. I will need to put a lot of effort here :/

I didn't really get this !? syntax

This is gentoo ebuild syntax. ? is conditional, ! is negation, so !?tool_verilator (fileset_tb) means to only include fileset_tb if not tool is set to verilator

I'm not sure about run vs build vs sim & Co.

With new flows (linting, formal etc) the old build and sim commands have some shortcomings. Original plan was to introduce run and remove the other two, but this turned out to be a bit awkward for bitstream generation, because it will try to program the bitstream directly after it's generated. So I will keep at least build (and probably sim for some time too). But you can basically see build and sim as special cases of run where build is run --target=synth --setup --build and sim is run --target=sim

Passing options to verilator seems to be broken, verilator_options doesn't end up in the config.mk of verilator.

It works for me (TM). Can you check if the verilator_options are set in the .eda.yml file? If you give me a link to the core file as well, I can look for errors there

imphil commented 6 years ago

Thanks for your reply Olof. As said before, the problem raised in this issue are probably solved and I can use it as a starting point to get Spyglass support into fusesoc. Thanks for the implementation!

Just a couple quick notes on the other topics:

run vs sim/build comments: I can live with whatever works (as long as I know what to type). Currently there seems to be rather large amount of inconsistency in the commands and sub-commands, especially given the mix of "old" and "new" ways of doing things. If you're looking for input in this regard it'd be first good to know what the plan actually is, as opposed to discussing an intermediate state which we have right now in the code base.

I didn't really get this !? syntax

This is gentoo ebuild syntax. ? is conditional, ! is negation, so !?tool_verilator (fileset_tb) means to only include fileset_tb if not tool is set to verilator

That syntax reminds me back of the ~good~ old perl/awk days. So tool_verilator is a flag that's magically set somehow? For me, this syntax feels very unusual and counter-intuitive, to say at least, and needs rather good documentation. (Or rethinking to make it more readable using explicit keywords like fileset_tb if tool == verilator)

olofk commented 6 years ago

Added some clarifications to your follow-up questions in the FuseSoC 1.8 release announcement

Closing now