Refactor Yosys for f4pga/symbiflow usage #335
Conversation
…tion in the Makefile
…tal variables" This reverts commit 454deb6.
|
I think we need to take another look at this. In my mind we should only need to have split_io being a boolean to select whether to run that extra step or not. We can't put the command itself as a tool_option, because the location will vary between different computers. However, if f4pga sets up something like, I donẗ know, (Speaking of finding... I couldn't find the split_io script now. Searched through the whole f4pga github organization without a single hit. Must be missing something. Could you point me to where the script is, so that I can take a look at it?) As for the other vars, |
|
Ok, so the script is called |
@olofk The reason I have the variables passed in via list instead of just triggered via boolean is that I really don't want to hardcode any values or even variables into Yosys, because F4PGA isn't stable enough in the long term and I'd rather have all that stuff inside the F4PGA flow. (i.g. the variable names might change, or the paths of the scripts might change, etc.)
On my install it's inside the scripts folder (
This has just now been updated in commit 2ab019b to remove [0], [1], and [2] since they can already be obtained or inferred.
split_io_list[4] and [5] are the args for the first and second calls to yosys, respectively. [6] and [7] are the variables needed for those calls, respectively. I think it is necessary for the variables to be passed in because they are unique to F4PGA and may change in the future. With commit 2ab019b these indexes are now [1], [2], [3], and [4] respectively (with [0] being the path of the python script). I can leave comments explaining the array values the Yosys tool class if you'd like, although it should become clear when it is implemented in the F4PGA flow. |
Oh, this is interesting. Looks like I need to update my F4PGA |
…(inside python module)
Commit 3c323d8 reflects the new way of accessing the split_inouts.py script, with the added benefit of reducing the number of passed in values by one |
|
@olofk @Pocketkid2 we are working on moving the python scripts which are currently distributed in the arch-defs tarballs to be shipped along with f4pga. At the same time, as @Pocketkid2 noted, we are making them usable by importing them in python or externally (either through the module or through command Nonetheless, |
|
Yes! |
|
@umarcor I saw a script running |
|
@Pocketkid2 I think things are starting to clear up now and that we can simplify a lot of things. One thing remaining is the env vars that we send to yosys in the last step after running EDIT: I remember now. It's in EDIT2: They all just run |
|
@olofk
So, we are in the middle of this process:
Therefore, if you want to implement this PR as if
Alternatively, you can use In any case, during this process, it would be really helpful if you let us know which utils/scripts/resources are being used by edalize "directly". Until now, nothing was explicitly public or private, so everyone was hardcoding the paths and manually plugging the scripts. As part of the cleanup process, we want to make it explicit which resources are public (expected to be called by others) and which are private (called by other scripts/functions within the f4pga package, but not externally). That will help us do refactorisations/cleanups in the future without breaking other projects. In the current implementation of
Yes. However, similarly to the shell scripts, we are also providing minimal functionality to avoid hardcoding the location of the TCL scripts. See https://github.com/chipsalliance/f4pga/blob/main/f4pga/wrappers/sh/xc7/synth.f4pga.sh#L21 and https://github.com/chipsalliance/f4pga/blob/main/f4pga/wrappers/sh/xc7/synth.f4pga.sh#L140. Or, if you are using it from Python: https://github.com/chipsalliance/f4pga/blob/9946ed7f3d13f3fc7d2a764a812894c5eeca668a/f4pga/flows/common_modules/synth.py#L25 and https://github.com/chipsalliance/f4pga/blob/9946ed7f3d13f3fc7d2a764a812894c5eeca668a/f4pga/flows/common_modules/synth.py#L97-L99. It will use envvar FPGA_FAM by default, but you can pass the arch/family as the second argument: https://github.com/chipsalliance/f4pga/blob/main/f4pga/wrappers/sh/quicklogic/synth.f4pga.sh#L124-L125. Overall, your questions about the envvars are very pertinent and I'd also like to understand it better. Until now, that huge |
@olofk Edit: Should clarify that |
|
@umarcor I'm extremely happy to hear about this cleanup. The current Symbiflow integration is very suboptimal because it uses those bash shell scripts instead of having Edalize call the tools directly, which means that we're missing out on most of the features Edalize can provide and the Edalize symbiflow code is a mess. I have tried myself a couple of times to dig through that wall of tcl/bash/python but never got further enough than to submit some tiny clean-up patch. I have been hoping for years to improve the symbiflow (now as f4pga) backend so I was very excited to see @Pocketkid2 taking on this task. Since we have been calling the shell scripts previously, we don't rely on any env vars or specific setups and I would prefer not to do that unless completely necessary. So I guess it's both good and bad that we're working on this in the middle of a big f4pga refactoring. The bad side is that things might not be completely ready yet, but the good thing is that we will be early adopters and can hopefully affect things to make the edalize integration even smoother. |
|
@Pocketkid2 I noticed that too about USE_LUT_CONSTANTS. I have a vague memory now that it was used in some cases when doing P&R for just a small part of the device. But I wonder if that's a feature more for the developers of the P&R tool than for the users. And in that case, I suspect the devs will not run through Edalize anyway. Searching through all of github I only find what looks like two test cases setting it, so I suggest dropping it actually. I think this means that the only items from EDIT: I see now that it uses the scripts in f4pga/wrappers/tcl/*/synth.f4pga.tcl. Those are reasonably complex so we should reuse them for now. Long-term I would like to split them up a bit so that the ordinary Edalize main tcl yosys file can just source them to get the extra stuff needed, but that's for another day. The thing we should look at here is what env vars these scripts expect and if we know the values of them without involving the user |
|
I mapped out all the env vars used in the various synth tcl scripts and it looks like this f4pga/wrappers/tcl/ice40/synth.f4pga.tcl: f4pga/wrappers/tcl/qlf_k4n8/synth.f4pga.tcl: f4pga/wrappers/tcl/xc7/synth.f4pga.tcl: I'm pretty sure most of them are just filenames that we can set as we wish. Some of them also seem to be alternative ways of loading extra files, which we don't need to support. |
|
@olofk I think we still need the conv.tcl script because it converts the I will go through all those variables and write the ones in Yosys that we reasonably can (a few of them do rely on f4pga install paths so those are still going to need some kind of path variable passed in) |
As shown in #323 (comment) symbiflow/f4pga has a lot of scripts, at different layers of abstraction. Most "consumers" call them as black boxes from the higher abstraction entrypoints, so there was no need to understand the internal details. However, that's a maintenance nightmare because the lack of explicit and organised documentation hits developers frequently. Using three different languages makes it more complex, because it's not comfortable to inspect/use variables/functions across them. As a result, it is not trivial for Edalize (or any other project) to call the tools "directly" and "once only", assuming that "the tools" are yosys and vpr. Both Xilinx and QuickLogic flows do have "patches" between stages/passes, and some of those patches depend on the envvars set in higher abstraction scripts, but it's not explicit. With regard to TCL scripts, I believe they exist because of the complexity to manage lists of lists, structs, etc. in bash. Originally, it was all done with makefiles and shell scripts only, so TCL was the obvious approach to put "complex" logic into the yosys pipeline. However, we are now dealing with it through Python. Hence, eventually, we can generate the specific sequence of commands with all hardcoded content, so that no TCL logic is needed. I.e., we might replace the current TCL scripts with templates. That will get rid of all the envvars in favour of explicit args with proper type checking. |
Yes, that's correct. The json format was invented by the yosys devs when they developed nextpnr but AFAIK no other tools use this format. Yosys can write blif directly, but since we need to do some extra hacks we first write out json, then do some extra operations on the json file (i.e. split_inouts) and then call yosys again to convert the hacked json to blif using conv.tcl. (This is also why I suggested running split_inouts directly from within yosys, so that we only need to call yosys once, and split_inouts would just be an extra line in synth.tcl. But let's not go there)
I believe the |
|
@umarcor I'm trusting your judgement on what would be the best layer of abstraction to call the EDA tools. Since Edalize is an abstraction layer we can always dig deeper later on if needed, but I really don't want to duplicate too much of what's inside f4pga. A sweet spot for me would be, as you mention, to e.g. call yosys directly from Edalize but using the f4pga synth scripts for all the device-specific quirks. Let's speak more about this at FPGA World in two weeks also :) |
|
Okay, so there's a couple of things here: First, I don't think you should depend on directly using Tcl scripts that are part of f4pga. Those scripts can (and will) change, including the environment variables. In fact, there's a PR that replaces direct references to environment variables with calls to a procedure that's internally provided by f4pga. To interact with f4pga, you should not be calling individual scripts. The fact that you can call (When running the tool from terminal): (When using f4pga directly in python): # These might get moved in the future
from f4pga.flows.commands import make_flow_config
from f4pga.flows.flow import Flow
from f4pga.flows.flow_config import ProjectFlowConfig
from f4pga.flows.common import set_verbosity_level
# Set amount of information on stdout (optional)
set_verbosity_level(2)
part_name = "XC7A35TCSG324-1"
# F4Cache object can be None. It's used for incremental builds.
f4cache = None
# Configure inputs and outpus
project_flow_cfg = ProjectFlowConfig("")
project_flow_cfg.flow_cfg = {
part_name: {
"dependencies": {
"sources": ["top.v"], # list of verilog sources
"eblif": "path/to/eblif/output.eblif" # optional, use it to override default output path of eblif
},
"values": {
"top": "top" # top module's name
}
}
}
# Build complete config using user's inputs and flow definitions provided by f4pga
flow_cfg = make_flow_config(project_flow_cfg, part_name)
# Build complete flow for target "eblif".
flow = Flow(target="eblif", cfg=flow_cfg, f4cache=f4cache)
# Execute the flow. Will produce the results or raise an exception.
flow.execute()In other words, either use yosys directly and you own copy of python and Tcl scripts, leaving f4pga uninvolved, or call f4pga to produce a desired results - in this case you could wrap In regards to |
|
@olofk Take a look at f37b08c and see what you think. I've gotten it down to just two single variables, neither of which can be resolved from information inside Yosys (input xdc constraints files used in the TCL scripts and the JSON part architecture file which can be resolved with device, part, and prjxray information). It should be noted that the F4PGA_SHARE_DIR environmental variable will be required, but this should be easy to point out or create automatically in F4PGA. |
edalize/tools/yosys.py
Outdated
| commands.add(command, targets, depends, variables=variables) | ||
|
|
||
| # Configure python script and additional call to Yosys | ||
| if split_io_list: |
There was a problem hiding this comment.
FYI, conv.tcl and executing split_inouts were merged into the synth.tcl script. A single call to yosys is enough now. See chipsalliance/f4pga#633.
There was a problem hiding this comment.
Oh, interesting, I will have to update my f4pga and take a look at that
|
@olofk What's the status on this? |
|
@kboronski-ant Is there any reason why you created a new project configuration format instead of reusing the EDAM format? I think it would be excellent to have that as an interchange format between Edalize, F4PGA and SiliconCompiler and in the long term I'm hoping to push support for it directly into the various EDA tools. And regarding using the f4pga python wrappers or calling the tools directly, I think there are benefits for Edalize to go directly. That means we can use the Edalize features that comes with all the other backends, like launcher files, tcl scripting and tool-specific escaping of defines/parameters/plusargs, etc. But at the same time it would be nice to not duplicate the synth tcl scripts, so having them available through a utils command would be great. A question. Have you considered instead to build the f4pga tooling on top of the Edalize EdaTool class? That was what I thought was the initial idea when I first heard about an f4pga refactoring. |
|
@Pocketkid2 It looks much better now! Happy to see things keep getting simplified. There are still too many env vars and I'm sceptical towards exposing the f4pga_synth tool option like that, but it shouldn't hurt anyone so I think we can leave it as is for now and move on to other things. The one thing I'm most worried about right now is having |
This had slipped my mind because most of what F4PGA supports right now is the xc7 stuff. Probably should fix that although I'll have to check with the latest versions of the architecture to see their paths (and if their paths have even been harmonized with the xc7 paths yet which was a big issue for me a month or two ago looking into supporting those other architectures with Edalize) |
|
@olofk 4f54526 should fix it for the time being, although on my install it's showing the eos-s3 techmap files being installed inside the xc7 environment, not sure if that's an issue on my end or one that f4pga is still working on. edit: I now realize that change won't actually really fix the problem until the f4pga install paths are harmonized (which is a known issue), so I reverted the commit and hope that you will allow it in like it is now for the present until we can properly fix it |
This reverts commit 4f54526. It turns out this change does not in fact fix the problem, so we will leave it like this for the time being
|
Yeah, I think we'll let it be for the time being. There were a lot of inconsistencies in symbiflow and f4pga is a moving target so we'll try to finish this up and revisit later when the dust has settled. Pulling this is now unless you have any other last-minute changes. Just let me know |
|
@olofk no I'm good, pull it in. If I have further tweaks to make they will be included in the PR that has the main F4PGA flow |
|
Woohoo! After some git fiddling it's merged now. Thank you for your contribution |
This contains the changes to Yosys which allows it to work with the F4PGA flow.
What F4PGA does is use its own TCL script for synthesis, then runs its own python script, and then runs another call to Yosys to generate the final
.ebliffile.All of the required values are passed in through an array. Because the TCL scripts look for environmental variables, they are in the passed-in array and are put before the command itself so as to avoid using environmental variables in the Makefile.
I'd love to get your thoughts on this. I know it looks messy and I would be happy to document it in whatever way you think proper. Ideally, this would only be used by the F4PGA flow.
On a side note, the array size can be reduced by taking advantage of the template variable for one of the script paths, which may help in preventing unnecessary template generation. I can take care of that if you like this PR as a whole and are willing to move forward with it.