Process XML tile tags #941

acomodi · 2019-08-28T14:20:11Z

Description

This PR is a follow up of #934. This changes allow to introduce in the XML architecture definitions the tile tags, which are relative to the newly introduced t_physical_tile_type. The reason to do so is to separate definitively the logical block types and the physical tile types.
As the current XML architectures are in a wrong format (as they do not contain the tiles tag), a new method has been added to upgrade_arch.py to easily upgrade the architectures following the format proposed in this PR.

Related Issue

This is a followup of #934 and is related also to this comment.

Motivation and Context

With the t_logical_block_type being separated from the t_physical_tile_type brings up the necessity of having also a different way of populating the two data structures. Therefore, with this PR, a new kind of top-level tag is added to the architecture definition, alongside with all the necessary changes to correctly parse the XML.

How Has This Been Tested?

Basic and Strong reg tests.

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

acomodi · 2019-08-28T14:58:51Z

@kmurray FYI.
I have a couple of issues with this part and is probably better to discuss them here:

There is no way to currently add an intermediate step to change the architectures on the fly when building on CI with the dev/upgrade_vtr_archs.sh script. As far as I have seen, this script is not included in any automatic process, therefore I believe that the architectures have to be manually changed by running this script and then pushed.
With all the architectures XML definitions upgraded with the script, all the vtr_reg_strong tests correctly succeed, except for the ones using the custom_grid architectures. It is curious that only these tests do fail. I have tried to find the failing point and this is what I got:

After the routing step, a segmentation fault occurs when adding the delay values to the rt_nodes. This happens exactly here. I have checked the possible reasons and it turns out that some of the rt_edges have a valid child, but uninitialized iswitches and next variables. By printing on output the rt_node subtree everything looked ok.
Here are two sample architecture definitions, one with the tile tags and the other which is the [
original xml.

I will try to provide more detailed information on this segfault, but for now do you have any suggestion on what could possibly be the cause?

acomodi · 2019-09-03T12:13:01Z

@kmurray
With the latest commit I have fixed the segfault issue: there was a problem with the port equivalences for which wrong values were assigned to the tile ports, producing a cascading effect that affected the routing step.

The only issue right now is to let CI change the architectures at run-time to enable the tile tags parsing, unless we want to update all the architectures accordingly (using the update_arch.py locally and than pushing the new architectures).

acomodi · 2019-09-04T14:41:42Z

For now I have added the upgrade_arch.py for each test in .travis.yml to test CI

acomodi · 2019-09-05T16:54:46Z

with 08a013c I have detached the pinloc assignments from the pb_graphs by moving the construction of the array directly in the read_xml_arch_file.cpp step given that pinlocations should not be bound to the information present in pb_graphs.

To do so I have also moved the token.h utility from vpr to libs/libvtrutil/

kmurray · 2019-09-16T21:13:19Z

1. There is no way to currently add an intermediate step to change the architectures on the fly when building on CI with the [`dev/upgrade_vtr_archs.sh`](https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/dev/upgrade_vtr_archs.sh) script. As far as I have seen, this script is not included in any automatic process, therefore I believe that the architectures have to be manually changed by running this script and then pushed.

That is correct, the convention we've followed is to manually run dev/upgrade_vtr_archs.sh whenever the architecture file format is changed.

Given that you've put the call to dev/upgrade_vtr_archs.sh in the CI here is what I'd recommend:

Let me complete the review & let you address any changes
Back-out the CI changes (so it doesn't call the arch upgrade script)
Manually run the upgrade script and check-in the updated architectures

acomodi · 2019-09-16T22:00:40Z

@kmurray All right, so I will proceed by adding to this PR also the architecture changes and back-out the CI changes after the first step is complete.

The update_arch.py script adds the tiles at the end of the architecture file and the indentation is kinda wrong. I am thinking to add the <tiles> tag between the <models> and the <complexblocklists>: what do you think?

Moreover, I guess that the documentation should change accordingly. I may update it as well.

kmurray

Overall this looks really good.

One thing I didn't see (perhaps I just missed it), is whether we check that the port types/names/pin-counts of the <tile> ports match those of the associated top-level <pb_type>. If we don't we should probably check that and error out if things are inconsistent, since that is the assumption we are making at the moment. (In the future that restriction may be lifted, but we should enforce it for now).

Also, since this touches the user-specified architecture file format it is important to update the architecture format documentation.

I've also included a couple of small comments below.

.travis.yml

libs/libarchfpga/src/physical_types.h

vtr_flow/scripts/upgrade_arch.py

kmurray · 2019-09-16T21:25:31Z

vtr_flow/scripts/upgrade_arch.py

+            <interconnect ... />
+        </pb_type>
+    </complexblocklist>
+


Excellent comment!

libs/libarchfpga/src/read_xml_arch_file.cpp

kmurray · 2019-09-16T21:29:12Z

libs/libarchfpga/src/read_xml_arch_file.cpp

+                    auto result = check_equivalence.insert(std::pair<t_physical_tile_type*, t_logical_block_type*>(&physical_tile, &logical_block));
+                    if (!result.second) {
+                        archfpga_throw(__FILE__, __LINE__,
+                                       "Logical and Physical types do not have a one to one mapping\n");


Good to see this check here!

libs/libarchfpga/src/read_xml_arch_file.cpp

acomodi · 2019-09-17T11:07:34Z

@kmurray Thanks for the review :)

I have solved the minor issues and also added a check to ensure that the ports do exactly match between pb_types and tiles (c112067).

I have also run dev/upgrade_archs.sh and enabled pretty_print. It caused many of the architectures to be reformatted, hence this commit introduces a pretty high number of line changes.
Maybe the bad thing about pretty print is that blank lines have been eliminated, and maybe this slightly reduces readability, but I believe that it is worth having a common format among all the test architectures.

acomodi · 2019-09-17T11:08:55Z

I will now proceed with adding a description of the changes in the documentation to address the new tiles tag addition.

kmurray · 2019-09-17T18:40:06Z

@acomodi That all looks good, only one last thing: check_port_equivalence() should also check that the pin equivalence setting is the same between the tile and pb_type ports (in addition to the port type and number of pins which are already checked). With that this should be good to merge!

Signed-off-by: Alessandro Comodi <[email protected]>

Detached the physical tile pinlocations from the pb_graph information. Now all the `custom` pinlocations pattern is built from the information present in the physical tile types only. Signed-off-by: Alessandro Comodi <[email protected]>

Signed-off-by: Alessandro Comodi <[email protected]>

- changed t_physical_port -> t_physical_tile_port - added strict port checks Signed-off-by: Alessandro Comodi <[email protected]>

Signed-off-by: Alessandro Comodi <[email protected]>

acomodi · 2019-09-17T19:06:10Z

@acomodi That all looks good, only one last thing: check_port_equivalence() should also check that the pin equivalence setting is the same between the tile and pb_type ports (in addition to the port type and number of pins which are already checked). With that this should be good to merge!

Sure, just updated the port check with the pin equivalence

kmurray · 2019-09-17T20:45:33Z

Thanks @acomodi. I think the next step is for you to start looking at supporting multiple sites per tile (as previously discussed).

acomodi force-pushed the add-tile-tags branch 2 times, most recently from eadd223 to b0f7717 Compare August 28, 2019 14:52

acomodi force-pushed the add-tile-tags branch 3 times, most recently from 03c9d1f to 8f1b870 Compare September 3, 2019 15:53

acomodi changed the title ~~WIP: process XML tile tags~~ Process XML tile tags Sep 3, 2019

acomodi force-pushed the add-tile-tags branch 3 times, most recently from 00c84c7 to d13c885 Compare September 3, 2019 16:52

probot-autolabeler bot added the libvtrutil label Sep 5, 2019

acomodi force-pushed the add-tile-tags branch from f20265d to 7d1c9c2 Compare September 11, 2019 09:38

probot-autolabeler bot added the docs Documentation label Sep 11, 2019

acomodi force-pushed the add-tile-tags branch from 7d1c9c2 to 159877c Compare September 12, 2019 10:37

kmurray reviewed Sep 16, 2019

View reviewed changes

acomodi force-pushed the add-tile-tags branch from 07de461 to 1795ac6 Compare September 17, 2019 11:01

acomodi force-pushed the add-tile-tags branch from 1795ac6 to c112067 Compare September 17, 2019 11:45

acomodi requested a review from kmurray September 17, 2019 13:39

acomodi added 17 commits September 17, 2019 20:58

initial addition of tile tag

3884a5e

Signed-off-by: Alessandro Comodi <[email protected]>

link physical/logical tiles with correct index

a92e4f9

Signed-off-by: Alessandro Comodi <[email protected]>

update upgrade_arch.py script and added initial regression test

0302f45

Signed-off-by: Alessandro Comodi <[email protected]>

run make format

4d47606

Signed-off-by: Alessandro Comodi <[email protected]>

fix port equivalence in physical tile ports

2fb7d2d

Signed-off-by: Alessandro Comodi <[email protected]>

added updating architectures stage in .travis.yml

972b6e8

Signed-off-by: Alessandro Comodi <[email protected]>

fix valgrind memory leak

5ddc7df

Signed-off-by: Alessandro Comodi <[email protected]>

run make format

f64a957

Signed-off-by: Alessandro Comodi <[email protected]>

fixed memory leaks

27ce0d0

Signed-off-by: Alessandro Comodi <[email protected]>

remove equivalent tile regression test

1caeb45

Signed-off-by: Alessandro Comodi <[email protected]>

addressed review comments

eda5c91

- changed t_physical_port -> t_physical_tile_port - added strict port checks Signed-off-by: Alessandro Comodi <[email protected]>

Fixing pretty-print by removing blank spaces in XMLs

7d18a9a

Signed-off-by: Alessandro Comodi <[email protected]>

backed-out dev/upgrade_archs.sh CI script

fb518ed

Signed-off-by: Alessandro Comodi <[email protected]>

updated all test architecture files with pretty-print and tile tags

8043c22

Signed-off-by: Alessandro Comodi <[email protected]>

Updated documentation to address the tiles tag addition

1fb36f1

Signed-off-by: Alessandro Comodi <[email protected]>

add port equivalence when checking ports of tiles and pb_types

012116e

Signed-off-by: Alessandro Comodi <[email protected]>

acomodi force-pushed the add-tile-tags branch from 8df4e15 to 012116e Compare September 17, 2019 19:04

kmurray merged commit 6041158 into verilog-to-routing:master Sep 17, 2019

acomodi deleted the add-tile-tags branch September 17, 2019 21:01

This was referenced Sep 19, 2019

Support Equivalent Placement Sites #513

Closed

Resource utilization with Equivalent Tiles #982

Closed

acomodi mentioned this pull request Oct 2, 2019

Equivalent sites #988

Merged

7 tasks

Process XML tile tags #941

Process XML tile tags #941

Uh oh!

Conversation

acomodi commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

acomodi commented Aug 28, 2019

Uh oh!

acomodi commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acomodi commented Sep 4, 2019

Uh oh!

acomodi commented Sep 5, 2019

Uh oh!

kmurray commented Sep 16, 2019

Uh oh!

acomodi commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmurray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kmurray Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kmurray Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acomodi commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acomodi commented Sep 17, 2019

Uh oh!

kmurray commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acomodi commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmurray commented Sep 17, 2019

Uh oh!

Uh oh!

acomodi commented Aug 28, 2019 •

edited

Loading

acomodi commented Sep 3, 2019 •

edited

Loading

acomodi commented Sep 16, 2019 •

edited

Loading

acomodi commented Sep 17, 2019 •

edited

Loading

kmurray commented Sep 17, 2019 •

edited

Loading

acomodi commented Sep 17, 2019 •

edited

Loading