Fix unroutability issues with new VPR #1711

acomodi · 2020-10-14T10:36:28Z

The newest VPR has fixed an issue with IPIN/OPIN RR nodes for which multiple RR nodes were emitted for the same site pin, due to the possible multiple side locations of the pin.

Therefore, with the new VPR, we cannot count on the presence of multiple nodes for the same pin during the routing import, to create the correct connections between the interconnection nodes and the site nodes.

This PR fixes this issue, assigning the same node ID to the directional graph nodes allowing to have multiple CHAN nodes connected to an IPIN/OPIN.

The corresponding VPR issue is the following one: verilog-to-routing/vtr-verilog-to-routing#1571.

This PR is expected to fail as with the current version of VPR, multiple nodes corresponding to the same site pin are emitted during the creation of the virtual RR graph.

acomodi · 2020-10-22T11:19:13Z

@litghost By removing the pres_fac 20.0 change and restoring it, XC7 CI went green. I have also tested it locally and everything passed, as well as vendor tools. I think that the reason CI went red was the pres_fac change.

litghost · 2020-10-22T14:33:07Z

@litghost By removing the pres_fac 20.0 change and restoring it, XC7 CI went green. I have also tested it locally and everything passed, as well as vendor tools. I think that the reason CI went red was the pres_fac change.

That makes sense. I'm going to work on fixing the vendor tool tests today, and after that we can re-run the CI here.

xc/common/utils/prjxray_routing_import.py

litghost · 2020-10-27T17:07:38Z

xc/common/utils/prjxray_routing_import.py

@@ -1019,6 +1023,20 @@ def get_number_graph_edges(conn, graph, node_mapping):
        if dest_graph_node not in node_mapping:
            continue

+        src_node, src_node_type = node_mapping[src_graph_node]


Need a comment on what this is going?

Consider factoring in a function?

Factored in a function and added a docstring to it

litghost · 2020-10-27T17:10:37Z

xc/common/utils/prjxray_routing_import.py

+            src_node_is_site_pin = src_node_type in pin_node_types
+            sink_node_is_site_pin = sink_node_type in pin_node_types
+
+            # It may happen that a same CHAN <-> PIN edge is generated and this is unaccepted


This is bad, because that means that the underlying rr graph is less connected than the true hardware routing graph. Also this code is too pessimistic. VPR supports allowing multiple CHAN <-> PIN edges if a different switch is used. So you need to account for that.

I have added an assertion to check whether the switch is the same for the same CHAN <-> PIN pair.

This was meant to avoid the error generated here

litghost

Comments in. This fix is good, but actually points out a break in the VPR data model, I've filled an issue here: verilog-to-routing/vtr-verilog-to-routing#1577

litghost · 2020-10-28T16:29:47Z

xc/common/utils/prjxray_routing_import.py

+    (CHAN, PIN) pair with the same switch.
+
+    It may happen that a same CHAN <-> PIN edge is generated and this is unaccepted
+    by VPR, as it allows only multiple edges between CHAN nodes.


The error you linked now accommodates multiple CHAN <-> PIN edges? So this comment is wrong?

Hmm, actually I think I found a bug in VPR, making this whole check useless. Testing it now

litghost · 2020-10-30T16:03:29Z

xc/common/utils/prjxray_routing_import.py

+                pin_name = pin.split(".")[-1]
+
+                if pin_name not in pin_sides_dict[tile_name]:
+                    pin_sides_dict[tile_name][pin_name] = list()


Should use a set rather than a list here?

Yes, makes sense, fixed.

litghost · 2020-10-30T16:04:00Z

xc/common/utils/prjxray_routing_import.py

+
+            pins = loc.text.split()
+            for pin in pins:
+                pin_name = pin.split(".")[-1]


Please add a comment showing an example of pin

acomodi · 2020-11-02T14:23:34Z

In particular, the issue seems to affect IO Pads.

TILE: RIOPAD_M, PIN: IOI_OLOGIC0_CLKDIV, NODE ID: 289862, X: 114, Y: 111                                                                                                                                                                                                        
     GRAPH_NODE: 2164301                                                                                                                                                                                                                                                                 
     GRAPH_NODE: 2164300                                                                                                                                                                                                                                                        
     GRAPH_NODE: 2164302

This pin node results in having three corresponding graph nodes (sides locations).
Each of the graph nodes ends up having the same RR node, and, during the edge import, the same RR node gets connected to a CHAN multiple times, generating the check_rr_graph_issue.

litghost · 2020-11-02T14:45:43Z

* Update the VTR master+wip (unless we re-introduce the non-averaging bug during the indexed data calculation, we would see the increased run-time).

How close are you to debugging the new base cost calculation? If it is going to be a bit, let's recut master+wip and put in this fix.

acomodi · 2020-11-02T14:58:01Z

@litghost I have updated the new base cost calculation results here. I think that it will require some more iterations to land, therefore I believe we might as well proceed by updating master+wip, possibly with a temporarily wip branch with the new base_cost calculation.

acomodi · 2020-11-12T17:10:01Z

@litghost All CIs went green. This fixes #1768

litghost · 2020-11-12T18:12:12Z

Ibex route time dropped, and CPD increased,

before:

path	pack time (sec)	place time (sec)	route time (sec)	t_crit (ns)	Fmax (MHz)
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/soc/ibex/ibex_arty/artix7-xc7a50t-virt-xc7a50t-test	35.16	34.79	98.66	27.7936	35.9795
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/soc/litex/mini_ddr_eth/minilitex_ddr_eth_arty/artix7-xc7a50t-virt-xc7a50t-test	166.09	125.58	259.44	21.2266	47.11070072
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/soc/litex/mini_ddr/minilitex_ddr_arty_a7/artix7_100t-xc7a100t-virt-xc7a100t-test	110.22	88.05	268.96	20.9075	47.82972617
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/soc/litex/mini_ddr/minilitex_ddr_arty/artix7-xc7a50t-virt-xc7a50t-test	116.94	100.81	231.37	20.6761	48.36502048
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/ddr/ddr_uart_arty100t/artix7_100t-xc7a100t-virt-xc7a100t-test	67.82	38.94	102.9	20.23	49.43153732
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/soc/litex/mini/minilitex_arty_a7/artix7_100t-xc7a100t-virt-xc7a100t-test	72.62	53.61	140.66	19.814	50.4693
/tmpfs/src/github/symbiflow-arch-defs-continuous-xc7/build/xc/xc7/tests/soc/litex/mini/minilitex_arty/artix7-xc7a50t-virt-xc7a50t-test	74.18	49.42	144.67	17.4073	57.4473

after:

path	pack time (sec)	place time (sec)	route time (sec)	t_crit (ns)	Fmax (MHz)
/tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/build/xc/xc7/tests/soc/ibex/ibex_arty/artix7-xc7a50t-virt-xc7a50t-test	35.65	36.69	85.54	30.7268	32.5449
/tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/build/xc/xc7/tests/soc/litex/mini_ddr_eth/minilitex_ddr_eth_arty_100t/artix7_100t-xc7a100t-virt-xc7a100t-test	150.1	104.89	323.12	22.2722	44.8990221
/tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/build/xc/xc7/tests/soc/litex/mini/minilitex_arty_a7/artix7_100t-xc7a100t-virt-xc7a100t-test	71.09	53.36	169.95	20.5686	48.6177
/tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/build/xc/xc7/tests/ddr/ddr_uart_arty100t/artix7_100t-xc7a100t-virt-xc7a100t-test	68.04	40.25	138.31	20.1271	49.68425655
/tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/build/xc/xc7/tests/soc/litex/mini_ddr/minilitex_ddr_arty_a7/artix7_100t-xc7a100t-virt-xc7a100t-test	110.74	86.1	276.85	19.9479	50.13059019
/tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/build/xc/xc7/tests/soc/litex/mini_ddr/minilitex_ddr_arty/artix7-xc7a50t-virt-xc7a50t-test	118.85	99.94	232.25	17.959	55.68238766

Theories?

acomodi · 2020-11-12T18:53:16Z

@litghost I'll need to look a closer look to this one.

One possibility might be that the different way we connect site PINs to CHAN could have caused this?

I cannot think about other reasons, as, apart from the different RR graph generated produced by VPR with one index for all the sides, there shouldn't have been other changes that influence the RR graph and the router.

litghost

I cannot think about other reasons, as, apart from the different RR graph generated produced by VPR with one index for all the sides, there shouldn't have been other changes that influence the RR graph and the router.

That's why I'm pointing this out. It surprised me :(

acomodi · 2020-11-13T14:28:08Z

@litghost By building the ibex test locally with a clean repo, using this PR, the ibex CPD I get is 27.887.

…eam. Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>

litghost · 2020-11-13T18:43:14Z

@litghost By building the ibex test locally with a clean repo, using this PR, the ibex CPD I get is 27.887.

Please rebase onto of #1772, and we'll re-examine the results.

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

acomodi · 2020-11-17T14:40:01Z

@litghost, with the bug with the non-deterministic ibex sv2v translation (fixed in #1782) and #1776 We cannot rely too much on results between different builds when comparing LiteX and Ibex designs.

All other tests are deterministic, therefore, from #1711 (comment) we can take as valid only the ddr_uart results.

We could compare other designs (e.g. scalable_proc) and, if everything looks fine and the code in this PR is ok, we could merge this, as it keeps holding the VtR+SymbiFlow CI going green.

acomodi force-pushed the fix-single-site-pin-node-issue branch from 111e664 to 79c2a42 Compare October 21, 2020 18:32

acomodi added the kokoro:force-run label Oct 21, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 21, 2020

acomodi changed the title ~~[WIP/DNM]: Fix unroutable issues with new VPR~~ Fix unroutable issues with new VPR Oct 22, 2020

acomodi added the kokoro:force-run label Oct 22, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 22, 2020

litghost added the kokoro:force-run label Oct 22, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 22, 2020

litghost added the kokoro:force-run label Oct 22, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 22, 2020

litghost added the kokoro:force-run label Oct 22, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 22, 2020

acomodi added the kokoro:force-run label Oct 26, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 26, 2020

acomodi requested a review from litghost October 27, 2020 10:05

acomodi changed the title ~~Fix unroutable issues with new VPR~~ Fix unroutability issues with new VPR Oct 27, 2020

litghost reviewed Oct 27, 2020

View reviewed changes

xc/common/utils/prjxray_routing_import.py Show resolved Hide resolved

litghost reviewed Oct 27, 2020

View reviewed changes

litghost suggested changes Oct 27, 2020

View reviewed changes

acomodi requested a review from litghost October 28, 2020 16:01

litghost reviewed Oct 28, 2020

View reviewed changes

acomodi mentioned this pull request Oct 29, 2020

check_rr_graph: fix bug when checking multi-connections between nodes verilog-to-routing/vtr-verilog-to-routing#1579

Merged

7 tasks

acomodi force-pushed the fix-single-site-pin-node-issue branch from 3fe9ba0 to 4578076 Compare October 30, 2020 08:52

acomodi added the kokoro:force-run label Oct 30, 2020

symbiflow-kokoro removed the kokoro:force-run label Oct 30, 2020

litghost reviewed Oct 30, 2020

View reviewed changes

acomodi mentioned this pull request Nov 5, 2020

New master+wip SymbiFlow/vtr-verilog-to-routing#549

Merged

acomodi force-pushed the fix-single-site-pin-node-issue branch from 69b9f39 to f41e3b9 Compare November 5, 2020 23:50

acomodi added the kokoro:force-run label Nov 6, 2020

symbiflow-kokoro removed the kokoro:force-run label Nov 6, 2020

acomodi added the kokoro:force-run label Nov 7, 2020

symbiflow-kokoro removed the kokoro:force-run label Nov 7, 2020

rw1nkler mentioned this pull request Nov 10, 2020

Add techmap for BUFGMUX #1419

Merged

acomodi added the kokoro:force-run label Nov 11, 2020

symbiflow-kokoro removed the kokoro:force-run label Nov 11, 2020

acomodi mentioned this pull request Nov 11, 2020

CI is broken #1768

Closed

acomodi force-pushed the fix-single-site-pin-node-issue branch from f41e3b9 to fa3e5be Compare November 12, 2020 08:43

acomodi requested a review from litghost November 12, 2020 09:10

litghost reviewed Nov 13, 2020

View reviewed changes

Update SymbiFlow/RapidWright fork instead of directly ingesting upstr…

376a73b

…eam. Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>

acomodi force-pushed the fix-single-site-pin-node-issue branch from fa3e5be to a419837 Compare November 13, 2020 18:45

rr_graph: use the same IPIN/OPIN node for site graph nodes

d299ba7

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

acomodi force-pushed the fix-single-site-pin-node-issue branch from a419837 to d299ba7 Compare November 13, 2020 18:46

rw1nkler mentioned this pull request Nov 17, 2020

Make Ibex sv2v conversion deterministic #1782

Merged

litghost merged commit 991c74f into f4pga:master Nov 17, 2020

litghost mentioned this pull request Nov 17, 2020

Add symbiflow-vtr-optimized with PGO enabled SymbiFlow/conda-packages#143

Closed

acomodi deleted the fix-single-site-pin-node-issue branch March 23, 2021 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unroutability issues with new VPR #1711

Fix unroutability issues with new VPR #1711

acomodi commented Oct 14, 2020 •

edited

acomodi commented Oct 22, 2020

litghost commented Oct 22, 2020

litghost Oct 27, 2020

litghost Oct 27, 2020

acomodi Oct 28, 2020

litghost Oct 27, 2020

acomodi Oct 28, 2020

litghost left a comment

litghost Oct 28, 2020

acomodi Oct 29, 2020

litghost Oct 30, 2020

acomodi Oct 30, 2020

litghost Oct 30, 2020

acomodi Oct 30, 2020

acomodi commented Nov 2, 2020

litghost commented Nov 2, 2020

acomodi commented Nov 2, 2020

acomodi commented Nov 12, 2020

litghost commented Nov 12, 2020 •

edited

acomodi commented Nov 12, 2020

litghost left a comment

acomodi commented Nov 13, 2020

litghost commented Nov 13, 2020

acomodi commented Nov 17, 2020

Fix unroutability issues with new VPR #1711

Fix unroutability issues with new VPR #1711

Conversation

acomodi commented Oct 14, 2020 • edited

acomodi commented Oct 22, 2020

litghost commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acomodi commented Nov 2, 2020

litghost commented Nov 2, 2020

acomodi commented Nov 2, 2020

acomodi commented Nov 12, 2020

litghost commented Nov 12, 2020 • edited

acomodi commented Nov 12, 2020

litghost left a comment

Choose a reason for hiding this comment

acomodi commented Nov 13, 2020

litghost commented Nov 13, 2020

acomodi commented Nov 17, 2020

acomodi commented Oct 14, 2020 •

edited

litghost commented Nov 12, 2020 •

edited