New F4PGA Flow

[Pocketkid2]

This is the first step in implementing #323. We created a new flow using the Edaflow system that runs the built-in Yosys/VPR tools (with some modifications). The module has been tested and works correctly on the Basys3 board, but should also work on other Xilinx 7 series boards that are supported with the f4pga tools.

[Pocketkid2]

@olofk With the Edalize flow definition (that consists of nodes), is there a way to inject regular commands like python scripts in between calls to a defined node such as Yosys or VPR? The F4PGA toolchain currently uses python scripts at multiple stages during synthesis and implementation and we're not sure if it's worth overhauling Yosys and VPR to account for that kind of thing right now.

[mithro]

@olofk With the Edalize flow definition (that consists of nodes), is there a way to inject regular commands like python scripts in between calls to a defined node such as Yosys or VPR? The F4PGA toolchain currently uses python scripts at multiple stages during synthesis and implementation and we're not sure if it's worth overhauling Yosys and VPR to account for that kind of thing right now.

My opinion is there are two parts here; (a) edalize needs to support these types of Python scripts. (b) We need to generally reduce the usage of these Python scripts in the F4PGA flow.

FYI - @kgugala / @acomodi

[Pocketkid2]

My opinion is there are two parts here; (a) edalize needs to support these types of Python scripts. (b) We need to generally reduce the usage of these Python scripts in the F4PGA flow.

This is a really good point. I feel that changing the F4PGA flow to remove the python might be beyond the scope of what I'm trying to do here, though

[Pocketkid2]

@olofk This PR is ready for review, it's stable. I moved the task list and project description to issue #323

[olofk]

Awesome! Thanks for working on this. I hope I can get around to review this soon, but please ping me if you don't hear back

[Pocketkid2]

@olofk What's the status on this?

[schafernc]

@olofk Sorry for bugging you when I'm sure you're busy, but I was wondering if you wouldn't mind reviewing this latest pull request. We would really like to move forward on this front. Thanks for all that you do!

[olofk]

Thank you for the work you have done. I have found some time to start reviewing now. Will let you know as soon as I have any comments

[olofk]

@Pocketkid2 My attack plan right now is to start by pulling in all independent smaller fixes and then it would be great if you could rebase and squash the remaining stuff.

If you're fine with this I'll start by cherry-picking https://github.com/olofk/edalize/pull/322/commits/07aed8542848e65587448fa697ec44f79f2b7cac https://github.com/olofk/edalize/pull/322/commits/2c3d5d25614e3180090df261610e57a7b891b222

After that I'll look at the changes to the individual tool classes, followed by the f4pga flow and finally the f4pga leacy API wrapper.

One more thing! After playing around with the new flow API I have discovered that some things didn't really work as intended. In particular, it was not possible to apply tool options for optional tools which made some use cases pretty useless. I think I have solved this now, but haven't pushed it yet, so take a look at https://github.com/olofk/edalize/pull/328 for my proposed fixes. I think those should preferably go in before any new flow (such as this one) is added

[Pocketkid2]

One more thing! After playing around with the new flow API I have discovered that some things didn't really work as intended. In particular, it was not possible to apply tool options for optional tools which made some use cases pretty useless. I think I have solved this now, but haven't pushed it yet, so take a look at #328 for my proposed fixes. I think those should preferably go in before any new flow (such as this one) is added

I got around this by creating the flow in the init function before calling super.init(), how does your change differ?

[Pocketkid2]

If you're fine with this I'll start by cherry-picking 07aed85 2c3d5d2

After that I'll look at the changes to the individual tool classes, followed by the f4pga flow and finally the f4pga leacy API wrapper.

Yup, take anything and everything you like. I apologize for stacking so many commits on this PR. Feel free to tell me about any changes I need to make.

[olofk]

One more thing! After playing around with the new flow API I have discovered that some things didn't really work as intended. In particular, it was not possible to apply tool options for optional tools which made some use cases pretty useless. I think I have solved this now, but haven't pushed it yet, so take a look at #328 for my proposed fixes. I think those should preferably go in before any new flow (such as this one) is added

I got around this by creating the flow in the init function before calling super.init(), how does your change differ?

I'm not sure how much it differs for the f4pga flow, perhaps it will be the same thing in practice. But the idea is that querying and setting options needs to be a two-stage approach. First get available flow options, then set them and query again which tool options are available for that particular flow configuration.

[olofk]

Alright. First two tiny commits added. Right now I'm looking at the nextpnr changes. I think we can make the diff much smaller if we do an elif here https://github.com/olofk/edalize/pull/322/files#diff-57dbc9962eed8258be08d24cc0ba97652eaff3c92c76d9c445be10002fc3cef0L92 instead of making a nested if. Or am I missing something?

Then I would like to talk about the chipdb files. I have a memory those were discussed last time we looked at nextpnr-xilinx, but can't find that discussion right now. My main question here is, who is supposed to distribute those? Are the users expected to know where they are and set the path to them, or do they come with the tools? A third alternative (which I think was my preference last time this was discussed), can we have some helper tool that can be queried for the location of these files? I made this one a while ago and I wonder if that was my answer to this problem. A secondary thing is that I'm reluctant to hog file_type : bin for these kind of files. There are a other types of files that might be a better fit for that file_type

[olofk]

This was the chipdb discussion I was thinking about. At least part of it. Still missing a user-friendly way to pull in the right ones without involving the user in the default case

[Pocketkid2]

Alright. First two tiny commits added. Right now I'm looking at the nextpnr changes. I think we can make the diff much smaller if we do an elif here https://github.com/olofk/edalize/pull/322/files#diff-57dbc9962eed8258be08d24cc0ba97652eaff3c92c76d9c445be10002fc3cef0L92 instead of making a nested if. Or am I missing something?

I admit I didn't structure this super well, I just wanted to get started with a working xilinx usage of nextpnr. I was planning on restructuring that better later but maybe we should do that now

[Pocketkid2]

Then I would like to talk about the chipdb files. I have a memory those were discussed last time we looked at nextpnr-xilinx, but can't find that discussion right now. My main question here is, who is supposed to distribute those? Are the users expected to know where they are and set the path to them, or do they come with the tools? A third alternative (which I think was my preference last time this was discussed), can we have some helper tool that can be queried for the location of these files? I made this one a while ago and I wonder if that was my answer to this problem. A secondary thing is that I'm reluctant to hog file_type : bin for these kind of files. There are a other types of files that might be a better fit for that file_type

Based on how much time it took me to figure out how to make the chipdb files myself I'm starting to think it would be nice to have a set of premade ones built in that correspond to officially supported boards. I'm actually working on a branch that does make use of an internal database that maps a simple board name to all its underlying part/device names and including default chipdb for nextpnr and default constraints file could easily go right in with that.

[olofk]

Then I would like to talk about the chipdb files. I have a memory those were discussed last time we looked at nextpnr-xilinx, but can't find that discussion right now. My main question here is, who is supposed to distribute those? Are the users expected to know where they are and set the path to them, or do they come with the tools? A third alternative (which I think was my preference last time this was discussed), can we have some helper tool that can be queried for the location of these files? I made this one a while ago and I wonder if that was my answer to this problem. A secondary thing is that I'm reluctant to hog file_type : bin for these kind of files. There are a other types of files that might be a better fit for that file_type

Based on how much time it took me to figure out how to make the chipdb files myself I'm starting to think it would be nice to have a set of premade ones built in that correspond to officially supported boards. I'm actually working on a branch that does make use of an internal database that maps a simple board name to all its underlying part/device names and including default chipdb for nextpnr and default constraints file could easily go right in with that.

I think we need to do a collective attempt at this. I have seen so many partial solutions and I'm guilty of one or two myself. Mapping boards to default constraints, devices etc is something that is found e.g. in Litex but also in many other places and I would love to see a unified database for this.

For this particular problem though, I think we need a map between device and where to find the corresponding chipdb file (my cache_and_query script also attempted to download them on the fly which is nice but not mandatory). If we can have have that, I don't think we need to have the chipdb option in nextpnr at all

EDIT: Just remembered that cache_and_query was a first prototype that I then expanded to handle the sky130 ASIC libraries which has kind of the same issue. The result is pdk-config which I also saw was mentioned in @umarcor's chart

[mithro]

FYI - https://github.com/hdl/constraints

[olofk]

FYI - https://github.com/hdl/constraints

Yes! That's likely the best source for this kind of data and I would highly recommend adding to that over keeping a separate board database. Still need something like pdk-config though to solve the chipdb location issue

[Pocketkid2]

@olofk What are the next steps to get this F4PGA flow merged? Do we need to take a step back and start simpler? Where should I start?

[Pocketkid2]

I think we can make the diff much smaller if we do an elif here https://github.com/olofk/edalize/pull/322/files#diff-57dbc9962eed8258be08d24cc0ba97652eaff3c92c76d9c445be10002fc3cef0L92 instead of making a nested if. Or am I missing something?

On second thought, I remember that the reason I specifically didn't use elif is because I wanted to add xilinx support to the same flow tool without disrupting existing usage or radically restructuring the code. The way nextpnr-xilinx operated in symbiflow isn't easily compatible with the default nextpnr commands.

[Pocketkid2]

Then I would like to talk about the chipdb files. I have a memory those were discussed last time we looked at nextpnr-xilinx, but can't find that discussion right now. My main question here is, who is supposed to distribute those? Are the users expected to know where they are and set the path to them, or do they come with the tools? A third alternative (which I think was my preference last time this was discussed), can we have some helper tool that can be queried for the location of these files? I made this one a while ago and I wonder if that was my answer to this problem. A secondary thing is that I'm reluctant to hog file_type : bin for these kind of files. There are a other types of files that might be a better fit for that file_type

The script you linked appears to be grabbing the VPR architecture files which I believe are already included in a default F4PGA install. (The symbiflow scripts had those paths as variables which I plugged into the Edalize makefile has environmental variables) I'm not sure if the same install by default includes nextpnr chipdb files or if there is an easy way for users to install them alongside their install. What I do know is that when I tested it, nextpnr-xilinx preferred .bin files to .bba files so that's what I started testing with.

[Pocketkid2]

FYI - https://github.com/hdl/constraints

Yes! That's likely the best source for this kind of data and I would highly recommend adding to that over keeping a separate board database. Still need something like pdk-config though to solve the chipdb location issue

I think we would still need an internal database for mapping board names to all of the required files and input variables, and this would just be how we access default constraints, right? And then if we want to add additional boards like the basys3 we could just add default constraints files to that repo

[Pocketkid2]

@olofk What's the status of your review? Any urgent revisions we should make before our flow can be merged in?

[olofk]

Sorry for the delay. Haven't been much near a computer lately. So, I think the best plan of attack is still to first plug in the individual tool classes, followed by the f4pga flow and finally the f4pga leacy API wrapper.

And for the tool classes, I think we could start with nextpnr as we have discussed. If that extra nested if is required then let's go with that, although looking at the code it seems to me like it would be totally possible to treat nextpnr-xilinx like the other nextpnr variants. Is there something in particular I'm missing?

Then the question still remain about the chipdb files. I really think nextpnr-xilinx should be able to figure this out by itself, or at least query the system where these files are located like the standard linux pkgconfig or yosys-config (or my own pdk-config which you pointed out is for vpr files that suffer from the same issue). Perhaps we should just use some environment variable for now that we can deprecate later when a real solution is found. Come to think of it, are the chipdb files specific for a device or does one file cover all supported devices? If it's the former, then I can't see any getting around the need for a device name to bba/bin file mapping? I'm fine with shipping such a script with Edalize for now if that helps though

Looking ahead, I think the split_io option needs to go. It looks like a list of options, but looking at the code, the four first elements are hard-coded to script, infile, outfile and endscript(?). I don't think it's feasible that users should need to input these things and I honestly don't know why the user should know about this split_io thing at all. I believe the Edalize backend should have all the necessary info to know when to insert this extra step by itself without involving the user.

For VPR I don't think we should need input_type. The VPR tool should just look at the file_type of the incoming netlist to discern what type it is.

There are probably some more things, but let's start with fixing the above

[Pocketkid2]

Then the question still remain about the chipdb files. I really think nextpnr-xilinx should be able to figure this out by itself, or at least query the system where these files are located like the standard linux pkgconfig or yosys-config (or my own pdk-config which you pointed out is for vpr files that suffer from the same issue). Perhaps we should just use some environment variable for now that we can deprecate later when a real solution is found. Come to think of it, are the chipdb files specific for a device or does one file cover all supported devices? If it's the former, then I can't see any getting around the need for a device name to bba/bin file mapping? I'm fine with shipping such a script with Edalize for now if that helps though

The reason why I'm a bit confused about this is because in my experience setting up nextpnr-xilinx on my machine, the chipdb files are compiled by the user manually and not pre-made or stored in some install directory. I could be wrong though, or maybe we just need to create that database.

[Pocketkid2]

And for the tool classes, I think we could start with nextpnr as we have discussed. If that extra nested if is required then let's go with that, although looking at the code it seems to me like it would be totally possible to treat nextpnr-xilinx like the other nextpnr variants. Is there something in particular I'm missing?

@olofk The reason I wrote the if-else is because the existing way of writing the nextpnr-xilinx did not work correctly with F4PGA, especially with the xilinx variant. If you're okay with me moving nextpnr over to the new way entirely I can rewrite that if-else to be a lot smoother.

[Pocketkid2]

Looking ahead, I think the split_io option needs to go. It looks like a list of options, but looking at the code, the four first elements are hard-coded to script, infile, outfile and endscript(?). I don't think it's feasible that users should need to input these things and I honestly don't know why the user should know about this split_io thing at all. I believe the Edalize backend should have all the necessary info to know when to insert this extra step by itself without involving the user.

I agree that this change needs to go, but I don't know how else to get the python script inbetween two calls of yosys. I can probably rework the yosys so that f4pga can use two yosys nodes but that will still leave us with a python script which I was hoping to leave in the flow until I could take the time to examine it more closely and move its functionality elsewhere. Do you have any ideas?

[Pocketkid2]

For VPR I don't think we should need input_type. The VPR tool should just look at the file_type of the incoming netlist to discern what type it is.

I'm not sure why I wrote it that way, possibly because I was confused at how VPR was reading the input file. Either way I'll go back and figure that out.

[olofk]

Then the question still remain about the chipdb files. I really think nextpnr-xilinx should be able to figure this out by itself, or at least query the system where these files are located like the standard linux pkgconfig or yosys-config (or my own pdk-config which you pointed out is for vpr files that suffer from the same issue). Perhaps we should just use some environment variable for now that we can deprecate later when a real solution is found. Come to think of it, are the chipdb files specific for a device or does one file cover all supported devices? If it's the former, then I can't see any getting around the need for a device name to bba/bin file mapping? I'm fine with shipping such a script with Edalize for now if that helps though

The reason why I'm a bit confused about this is because in my experience setting up nextpnr-xilinx on my machine, the chipdb files are compiled by the user manually and not pre-made or stored in some install directory. I could be wrong though, or maybe we just need to create that database.

I have zero experience with nextpnr-xilinx myself but I got the impression that there were pre-compiled files somewhere that users were supposed to use, but it might very well be that this is still experimental. Either way, let's go with your solution of having users provide the chipdb file manually for now and hopefully the backend (or nextpnr itself) can eventually bring in the right files by itself. I do want to use the file type chipdbinstead of bin or bba since that seems to better describe what kind of file it actually is. With that change I'm fine with the nextpnr patches (no need to change the if-elsething. Let's revisit that later if needed). Could you please open a new PR with only the nextpnr changes and I can pull that in right away

[olofk]

Looking ahead, I think the split_io option needs to go. It looks like a list of options, but looking at the code, the four first elements are hard-coded to script, infile, outfile and endscript(?). I don't think it's feasible that users should need to input these things and I honestly don't know why the user should know about this split_io thing at all. I believe the Edalize backend should have all the necessary info to know when to insert this extra step by itself without involving the user.

I agree that this change needs to go, but I don't know how else to get the python script inbetween two calls of yosys. I can probably rework the yosys so that f4pga can use two yosys nodes but that will still leave us with a python script which I was hoping to leave in the flow until I could take the time to examine it more closely and move its functionality elsewhere. Do you have any ideas?

This was an attempt at a first step towards untangling the python/tcl/bash interdependencies in symbiflow but it looks like it was never merged. The original idea with that PR was to run split_io script from tcl script launched by yosys so that we only needed to run yosys once. Unfortunately I lost the battle against CMake in SymblFlow so the ambitions of the aformentioned PR ended up just covering a first step towards that goal.

Thinking about it again now I think perhaps we should choose another route and add in the split_io script and the extra yosys invocation in flows/f4pga.py instead since it's really a flow-specific hack and no other flow will likely have any use of that.

[Pocketkid2]

Could you please open a new PR with only the nextpnr changes and I can pull that in right away

@olofk Done.

[Pocketkid2]

Thinking about it again now I think perhaps we should choose another route and add in the split_io script and the extra yosys invocation in flows/f4pga.py instead since it's really a flow-specific hack and no other flow will likely have any use of that.

@olofk I saw your other comment about the VPR needing to be patched. If you want to go ahead and merge your VPR patch, then I'm happy to create another new PR with just the VPR changes on top of your patch, perhaps with a simpler way of triggering the intermediate python script (Maybe a boolean with the variables hardcoded in the VPR tool instead of an array of strings representing those variables since nobody will really need to know those details?).

[olofk]

Thinking about it again now I think perhaps we should choose another route and add in the split_io script and the extra yosys invocation in flows/f4pga.py instead since it's really a flow-specific hack and no other flow will likely have any use of that.

@olofk I saw your other comment about the VPR needing to be patched. If you want to go ahead and merge your VPR patch, then I'm happy to create another new PR with just the VPR changes on top of your patch, perhaps with a simpler way of triggering the intermediate python script (Maybe a boolean with the variables hardcoded in the VPR tool instead of an array of strings representing those variables since nobody will really need to know those details?).

Great. I'll start by pulling in my patch to fix VPR and you can add your changes on top after that. I hadn't thought about putting the split_io stuff in the vpr instead of the yosys class before, but that's actually more logical since it's VPR that requires a special blif version. Let's see if we can make that work

[Pocketkid2]

Great. I'll start by pulling in my patch to fix VPR and you can add your changes on top after that. I hadn't thought about putting the split_io stuff in the vpr instead of the yosys class before, but that's actually more logical since it's VPR that requires a special blif version. Let's see if we can make that work

I think I was actually mixed up when I wrote that, it's the generate_constraints script that gets used during VPR and not split_io. Is that change you suggest even possible without actually modifying the script? Because if so, that might be something I have to work with the F4PGA people on.

[Pocketkid2]

Thinking about it again now I think perhaps we should choose another route and add in the split_io script and the extra yosys invocation in flows/f4pga.py instead since it's really a flow-specific hack and no other flow will likely have any use of that.

@olofk I see what you are saying now, so we should make some other change to allow a python node to be added at the flow level? I probably will still have to make some changes to yosys to allow for a certain tcl script/command line argument to be used