Skip to content

WIP: Equivalent tiles VTR feature #36

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 16 commits into from
Closed

WIP: Equivalent tiles VTR feature #36

wants to merge 16 commits into from

Conversation

acomodi
Copy link
Collaborator

@acomodi acomodi commented Apr 1, 2019 •
edited
Loading

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

This is a very preliminar WIP to add the equivalent tiles feature to VPR.

Description

The equivalent tiles will let the placer to choose more freely the location of certain blocks that can be placed in different tiles (e.g. SLICELs could be implemented both on a SLICEM or SLICEL). These changes will add a freedom level to the placer.

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 acomodi force-pushed the equivalent-tiles branch 2 times, most recently from a32157c to b4aed55 Compare April 1, 2019 16:00
@acomodi acomodi requested review from litghost and mithro April 1, 2019 17:56
litghost
litghost reviewed Apr 1, 2019
View reviewed changes
Copy link

@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.

Not sure there is much to review here? This appears to just add parsing support without actually changing the placer? Or is this all there is?

@acomodi
Copy link
Collaborator Author

acomodi commented Apr 1, 2019

Not sure there is much to review here? This appears to just add parsing support without actually changing the placer? Or is this all there is?

Actually, for now, there's only the parsing logic. I am currently going on with the placer changes. The review request was just to check whether this is the right direction to go :)

@litghost
Copy link

litghost commented Apr 1, 2019

Not sure there is much to review here? This appears to just add parsing support without actually changing the placer? Or is this all there is?

Actually, for now, there's only the parsing logic. I am currently going on with the placer changes. The review request was just to check whether this is the right direction to go :)

Without seeing the placer changes and seeing how you are handling the routing connections, I cannot really provide input. I see nothing grossly wrong, but I don't see the solutions to the hard part of the problem either.

@acomodi acomodi removed the request for review from mithro April 1, 2019 18:15
@acomodi
Copy link
Collaborator Author

acomodi commented Apr 1, 2019

@litghost Right, too early for a review. When there will be something more concrete I'll ask for another one then

@acomodi acomodi force-pushed the equivalent-tiles branch 4 times, most recently from 638e72c to 9708bbc Compare April 4, 2019 15:50
@acomodi
Copy link
Collaborator Author

acomodi commented Apr 10, 2019 •
edited
Loading

I have cleaned up the workarounds and managed to have a cleaner solution. The pin mapping management seems to be working fine, the equivalent_tiles regression test that I have added is passing. I didn't yet push the placer changes given that I wanted to solve the pin issue first.

I still need to handle the situation in which the capacity of an equivalent tile changes (e.g. in the BRAM cases).

I will proceed now with the modification of the placer.

@acomodi acomodi requested review from litghost and mithro April 10, 2019 09:59
@acomodi
Copy link
Collaborator Author

acomodi commented Apr 10, 2019 •
edited
Loading

Placer now can choose initial locations among all the available tiles (original and equivalent tiles) for a block.

I still need to handle the swap algorithm to choose equivalent tiles if it results in a minor cost and a better timing.

(CI is failing because all the XMLs require the <tiles> tag now)

This was referenced Apr 12, 2019
Closed
Closed
litghost
litghost requested changes Apr 15, 2019
View reviewed changes
@acomodi acomodi requested a review from litghost May 6, 2019 14:11
litghost
litghost requested changes May 6, 2019
View reviewed changes
Copy link

@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.

Looking good. I recommend you start this PR on the upstream to begin to get their feedback.

libs/libarchfpga/src/read_xml_arch_file.cpp Outdated
+ Type->pb_type->num_clock_pins);
Type->num_receivers = Type->capacity * Type->pb_type->num_input_pins;
Type->num_drivers = Type->capacity * Type->pb_type->num_output_pins;
Type->index = i;
Copy link

Choose a reason for hiding this comment

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

Tile type index should be StrongId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just checked it, in t_type_descriptor the index type is int. I could change it to StrongId though

Copy link

Choose a reason for hiding this comment

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

Ask VTR devs about it. Seems weird to have random integer ID's floating around. Might be an old example.

libs/libarchfpga/src/read_xml_arch_file.cpp Outdated
pugi::xml_node CurType;

Type->num_equivalent_tiles = count_children(Node, "mode", loc_data);
Type->equivalent_tiles = (t_type_descriptor **) vtr::malloc(Type->num_equivalent_tiles * sizeof(t_type_descriptor *));
Copy link

Choose a reason for hiding this comment

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

Please just use std::vector

Copy link

Choose a reason for hiding this comment

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

See verilog-to-routing#506

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, I have converted all the new members related to equivalent tiles (e.g. pin_mapping as well) to unordered_maps.
I have used unordered_maps as I need to access to elements by indices and vectors are not too suited for index access.

vpr/src/place/place.cpp Outdated
@@ -230,7 +230,7 @@ static int try_place_macro(int itype, int ipos, int imacro);
static void initial_placement_pl_macros(int macros_max_num_tries, int * free_locations);

static void initial_placement_blocks(int * free_locations, enum e_pad_loc_type pad_loc_type);
static void initial_placement_location(int * free_locations, ClusterBlockId blk_id,
static void initial_placement_location(int * free_locations, int itype,
Copy link

Choose a reason for hiding this comment

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

int itype should be a StrongId

@acomodi
Copy link
Collaborator Author

acomodi commented May 6, 2019

Looking good. I recommend you start this PR on the upstream to begin to get their feedback.

Ok, I'll deal with the requested changes and than open the PR upstream

acomodi added 7 commits June 27, 2019 14:46
@acomodi
equivalent tiles: added equivalent tile placement capability
9814291 
This commit introduces two major features:
- introduce the tile concept in the architecture XML. Top level pb_type
is discarded and all the top level pb_type information are moved into
the tile tags. This will cause CI build to fail as all the architectures
do not include the tiles tag right now.

- introduce the possibility to place blocks in equivalent tiles (SLICEL
blocks into SLICEM ones). According to the XML architecture description
there could be tiles equivalent to others that can be used during the
placement step (this can bring to better placement solutions)

Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
vtr_flow: added script to add tiles to architecture xml
980dc67 
I have also changes travis.yml to install the lxml python package
needed by the script

Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
vtr_flow: added comment
bc50eee 
Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
ODIN II: regression script modifies arch.xml with tiles
7637657 
Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
vtr_flow: added tiles step to upgrade arch
15ebc13 
Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
ODIN_II: corrected arch.xml file generation
e1200cd 
Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi
vpr: corrected bug in equivalent placement
a59408a 
Signed-off-by: Alessandro Comodi <[email protected]>
@acomodi acomodi force-pushed the equivalent-tiles branch from 90a87b9 to a59408a Compare June 27, 2019 12:48
@acomodi acomodi force-pushed the master+wip branch 2 times, most recently from b58ffb4 to 8e03da9 Compare July 5, 2019 10:21
@acomodi acomodi force-pushed the master+wip branch 4 times, most recently from 0195849 to a5b8bf6 Compare July 29, 2019 11:06
@litghost litghost force-pushed the master+wip branch 6 times, most recently from dc80177 to ed1d247 Compare August 5, 2019 21:26
@acomodi acomodi force-pushed the master+wip branch 2 times, most recently from 31a37cc to 4e20176 Compare August 30, 2019 15:05
@litghost litghost closed this Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mithro mithro Awaiting requested review from mithro

@litghost litghost Awaiting requested review from litghost

Assignees
No one assigned
Labels
infra lang-cpp lang-perl lang-python lang-shell libarchfpga Odin scripts tests VPR VTR Flow
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@acomodi @litghost @mgielda