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

Heterogeneous tiles implementation #1386

Merged
merged 16 commits into from
Apr 7, 2020
Merged

Conversation

acomodi
Copy link
Contributor

@acomodi acomodi commented Mar 30, 2020

Signed-off-by: Alessandro Comodi acomodi@antmicro.com

This PR is an initial implementation of the heterogeneous tiles introduced with verilog-to-routing/vtr-verilog-to-routing#1208.

TODO:

  • find a better way to use the start_tile implementation so that it gets used more generally by the various tile import scripts.
  • produce a new VtR master+wip with the heterogeneous tiles implementation

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@probot-autolabeler probot-autolabeler bot added arch-artix7 lang-python Issue uses (or requires) Python language. type-utils Issues is related to the scripts inside the repo. labels Mar 30, 2020
@acomodi acomodi requested a review from litghost March 30, 2020 16:49
@acomodi
Copy link
Contributor Author

acomodi commented Mar 30, 2020

Update: I am testing this PR alongside with the VtR heterogenous implementation integrated in master+wip and the litex-linux base test seems to be working, meaning that it passes Memtest.

I need to setup locally a tftp server to load linux binaries in DDR

@litghost
Copy link
Contributor

Update: I am testing this PR alongside with the VtR heterogenous implementation integrated in master+wip and the litex-linux base test seems to be working, meaning that it passes Memtest.

I need to setup locally a tftp server to load linux binaries in DDR

Awesome!

@acomodi
Copy link
Contributor Author

acomodi commented Mar 31, 2020

Edit: Linux boots correctly. I used UART to load binaries as I have some troubles with the tftp server (on the local host, so unrelated to the bitstream).

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi acomodi changed the title heterogeneous tiles: initial implementation Heterogeneous tiles implementation Apr 1, 2020
@acomodi acomodi requested a review from litghost April 1, 2020 14:10
utils/lib/pb_type_xml.py Outdated Show resolved Hide resolved
utils/lib/pb_type_xml.py Outdated Show resolved Hide resolved
utils/lib/pb_type_xml.py Outdated Show resolved Hide resolved
utils/lib/pb_type_xml.py Outdated Show resolved Hide resolved
Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Waiting for CI results. I want to compare quality and runtime results. Overall looks great!

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
Copy link
Contributor Author

acomodi commented Apr 1, 2020

For some reason the vendor tool buildfailed. I think though, by looking at the log, that all the tests acutally passed.

@litghost
Copy link
Contributor

litghost commented Apr 1, 2020

For some reason the vendor tool buildfailed. I think though, by looking at the log, that all the tests acutally passed.

There was a failure, but I don't think it is related.

@acomodi
Copy link
Contributor Author

acomodi commented Apr 6, 2020

I have run the bram_test_36 on test on symbiflow master against a local master+wip version with the updated master and explicit_ports and it seems to correctly route the constant net.

This indicates that there might be something not correctly working with:

  • The heterogeneous tiles VtR implementation: I do not think this is the case, because there is nothing in the heterogeneous tiles that changes routing behaviour. One thing to check though is the correctness of the post-placement synchronization of the net terminals, which should be fine though as, if they weren't, there would most probably be a VTR assertion failure. In addition, the BRAM tile has only one equivalent site as well as only one sub tile;
  • Changes introduced with this PR
  • ??

@acomodi
Copy link
Contributor Author

acomodi commented Apr 6, 2020

@litghost I realized one other thing that is different w.r.t. symbiflow master is that we are now using the bucket router_heap. I have tried to remove it and use the default one and, at least locally, the failing bram_36_diff_fasm test passes correctly.

I think that might be the reason for the vendor tools to fail. Anyhow, I am unsure whether this is a strong failure, meaning that it could potentially be solved in fasm2bels, as probably, for this weird route through two FAN pips, the tcl won't include the fixed route to the REGCLKB site pin of the BRAM 36.

@mkurc-ant mkurc-ant mentioned this pull request Apr 6, 2020
@litghost
Copy link
Contributor

litghost commented Apr 6, 2020

we are now using the bucket router_heap

We were using the bucket heap before, the difference is the default router heap was set to the bucket heap, and now it matches upstream, and the default router heap is binary again. See https://github.com/SymbiFlow/vtr-verilog-to-routing/pull/462/files#diff-f021ce5c0df49c7bf6020d0a4d94db66L1720

GitHub
Included branches: wip/heterogeneous_tiles Deleted branches: wip/add_explicit_ports wip/refactor_heap3

return tile_xml


def start_tile(
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for splitting the functions, I think it looks much clearer.

#
# Future support: Emit tiles with capacity that contain more than 1 site type
# once VPR supports it.
# SITE_TYPES: contains the comma separated list of sites that are going to be used
Copy link
Contributor

Choose a reason for hiding this comment

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

Because SITE_TYPES is now a mutliValueArgs, this is simply a CMake list

@acomodi
Copy link
Contributor Author

acomodi commented Apr 6, 2020

@litghost Indeed, what I meant was that by leaving the default heap (which is the binary with the new master+wip) the vendor tools pass.

What remains to understand is why using the bucketed one (still to verify with CI though) the bram_test_36 fails.

@litghost
Copy link
Contributor

litghost commented Apr 6, 2020

@litghost Indeed, what I meant was that by leaving the default heap (which is the binary with the new master+wip) the vendor tools pass.

I expect randomness? I doubt the default heap always passes. It is worth making sure the bucket heap is completing faster than the binary heap, and by a significant amount. If the binary heap is 1% slower, but always has a green CI, that is not the same situation as a 10-15% slower, right?

@acomodi
Copy link
Contributor Author

acomodi commented Apr 6, 2020

@litghost I would say we let this CI run finish and extract run-time data. As I suppose, the bucket heap should give us greater performance, in that case we may consider solving the weird jump on the FAN_BOUNCE in fasm2bels?
What I have experienced is that, for several runs, the bucket heap always failed that test, instead the binary one did succeed everytime.

@litghost
Copy link
Contributor

litghost commented Apr 6, 2020

in that case we may consider solving the weird jump on the FAN_BOUNCE in fasm2bels?

No this should be fixed in either fasm2frames or before fasm2frames.

@acomodi
Copy link
Contributor Author

acomodi commented Apr 6, 2020

@litghost All right, then is it worth restoring the bucket heap (if it results to be required to maintain the great improvement w.r.t. the binary one), removing the diff fasm test from the bram_test_36 and opening an issue to keep track of this?

@litghost
Copy link
Contributor

litghost commented Apr 6, 2020

@litghost All right, then is it worth restoring the bucket heap (if it results to be required to maintain the great improvement w.r.t. the binary one), removing the diff fasm test from the bram_test_36 and opening an issue to keep track of this?

To be clear, I don't believe switching from the binary to bucket or vise-versa actually fixes the vendor tools test. However if the vendor tools test is consistently green on binary, then let's do that.

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@litghost
Copy link
Contributor

litghost commented Apr 7, 2020

If CI is green, merge

@acomodi
Copy link
Contributor Author

acomodi commented Apr 7, 2020

xc7 vendor tools went green even though GH does report it as still running. Merging.

@acomodi acomodi merged commit 8f0cbe0 into f4pga:master Apr 7, 2020
@acomodi acomodi deleted the heterogeneous branch July 14, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-artix7 lang-python Issue uses (or requires) Python language. type-utils Issues is related to the scripts inside the repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants