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.

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

Add extended lookahead map #1449

Merged
merged 18 commits into from
Sep 11, 2020
Merged

Add extended lookahead map #1449

merged 18 commits into from
Sep 11, 2020

Conversation

acomodi
Copy link
Collaborator

@acomodi acomodi commented Jul 23, 2020

Description

This PR adds a new lookahead map method called extended map. This new kind of lookahead takes the same mechanisms of generation of the cost map as found in the connection box based lookahead currently implemented in the SymbiFlow/VTR fork, and uses the OPIN -> CHAN and CHAN -> IPIN delay information from the upstream lookahead map.

This PR includes new code to perform the node sampling which is more robust with respect to the current implementation, but takes more time to compute, hence the need of parallelize the graph exploration to fill the map costs.

Even with parallelization, though, the lookahead generation takes too long for the current regression tests in VTR, therefore the choice of adding a completely new lookahead type (extended map), so not to interfere with the current implementation.

Related Issue

There is no issue currently open, but this PR is one of the last ones needed to fully upstream the SymbiFlow/VTR fork's changes needed to perform P&R using upstream VTR for 7-series devices.

Motivation and Context

The current lookahead map implementation is not sufficient to provide accurate estimates during the router expansions for 7-Series devices. This results in high run-times with bad CPD results. To achieve better results, the lookahead build strategy needed to be improved (at least for 7-series).

How Has This Been Tested?

The new lookahead has been tested on SymbiFlow architectures: the issue keeping track of the progress is this one

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

@probot-autolabeler probot-autolabeler bot added build Build system lang-cpp C/C++ code lang-make CMake/Make code VPR VPR FPGA Placement & Routing Tool labels Jul 23, 2020
@acomodi acomodi mentioned this pull request Jul 23, 2020
Open
@acomodi acomodi requested review from vaughnbetz, litghost and HackerFoo and removed request for HackerFoo July 23, 2020 14:09
@probot-autolabeler probot-autolabeler bot added the libarchfpga Library for handling FPGA Architecture descriptions label Jul 27, 2020
@acomodi acomodi force-pushed the add-extended-lookahead-map branch from 6586087 to 88b5632 Compare July 27, 2020 19:46
@acomodi acomodi changed the title Add extended lookahead map WIP: Add extended lookahead map Jul 27, 2020
@vaughnbetz
Copy link
Contributor

Not sure what all the failures are due to -- rerun?

@vaughnbetz
Copy link
Contributor

Looks like at least some of these failures are due to failures in the new lookahead builder.

@acomodi
Copy link
Collaborator Author

acomodi commented Aug 4, 2020

I have restored the changes to only add the extended lookahead map. Turns out that, even with the bugfix of the CHANX/CHANY that got merged together, CPD and in general QoR seems to be worsening also for the strong regression tests.

The problem is that the lookahead map and the extended lookahead map use two very different methods of both generating and querying the cost map.

I am unsure whether the two methods can be merged together safely and preserve the QoR of vtr regression tests, as well as run-time.

This was referenced Aug 4, 2020
Closed
Closed
@acomodi acomodi changed the title WIP: Add extended lookahead map Add extended lookahead map Aug 10, 2020
@acomodi
Copy link
Collaborator Author

acomodi commented Aug 11, 2020

I believe this is ready for review at the current state.

I have exported some of the code from the lookahead map in the lookahead_map_utils file as it could be shared with the extended map code.

This implementation does not interfere with the regression tests as they all use the map lookahead.
I might as well add some tests which do make use of the extended map to test its behaviour and avoid regressions as well.

@probot-autolabeler probot-autolabeler bot added the VTR Flow VTR Design Flow (scripts/benchmarks/architectures) label Aug 11, 2020
@acomodi
Copy link
Collaborator Author

acomodi commented Aug 11, 2020

@vaughnbetz @litghost @HackerFoo FYI, I think this PR is ready for a first review iteration

litghost
litghost approved these changes Aug 11, 2020
View reviewed changes
Copy link
Collaborator

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

LGTM

@acomodi acomodi force-pushed the add-extended-lookahead-map branch 2 times, most recently from a8e9b8c to 53bfad8 Compare September 10, 2020 17:22
@acomodi
Updates to lookahead map.
2dc1a34 
  - New sampling method for lookahead map.
  - New spiral hole filling algorithm
  - Use a lightweight version of PQ_Entry (PQ_Entry_Lite)
  - Use maps in run_dijkstra()
  - Move context of router_lookahead_map_utils inside of util
    namespace.
  - Parallelization of lookahead generation

lookahead: add new extended map lookahead

This lookahead generates a map with a more complex and complete way of
sampling and expanding the nodes to reach all destinations.

The dijkstra expansion is parallelized to overcome the higher number of
samples needed.

It also adds a penalty cost factor based on distance between the origin
and destination nodes.

extended_lookahead: cleaned code and reduced code duplication

Signed-off-by: Dusty DeWeese <dustin.deweese@gmail.com>
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
tests: added extended_map to router_lookahead strong test
99d3b94 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
context: removed not needed segment_inf member from the device ctx
53f2dd1 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead cost map: refactor and commented
c1d21d3 
- The cost map filling routine has been simplified and the relevant code
extracted in two different sub-routines (penalty calculation, cost
filling)
- Commented used Doxygen compatible style some of the important methods
and class members

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
extended_lookahead: fix SRC/OPIN delay
894ec9f 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
extended_map: added in-code comments
2472366 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead_map_utils: added comments
c254627 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead: wider usage of RRNodeId where possible
b3e6f83 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
run make format
98e56f8 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead_cost: avoid building the segment map
d9d1ed8 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
address review comments
aefe2f1 
- Added better description on the user options
- Added test with random_shuffle nodes reordering

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead_sampling: simplified find_sample_regions code
39a5524 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
reg_test: added missing golden results
392996a 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
extended_lookahead: re-enable SRC/OPIN lookahead cost
070bb74 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead: addressed review comments
98cc4d8 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead: added comment to the special segment type constant index
f4b7155 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
route_timing: remove un-necessary headers
fb214f2 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
lookahead: fixed rebase conflicts
3111500 
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@acomodi
Copy link
Collaborator Author

acomodi commented Sep 11, 2020

@vaughnbetz I have rebased and solved all the conflicts due to the RCV merge

@vaughnbetz
Copy link
Contributor

vaughnbetz commented Sep 11, 2020 •
edited

Thanks @acomodi . There are a few unresolved conversations, requests.

  1. Can you go through and comment on them: either change (mostly commenting and changing to RRNodeIDs) or make a new PR/issue to track those updates so we don't lose them?
  2. Waiting for kokoro to finish.

@acomodi : if you can do 1 (comment / transfer to new PR or issue) while we're waiting for 2 (kokoro) then I can merge once kokoro finishes.

@acomodi
Copy link
Collaborator Author

acomodi commented Sep 11, 2020

@vaughnbetz Ok, I will open a new PR based on this one to add the additional comments and the assertion, as well as opening the issues.

I do not have time unfortunately to update the RRNodeId today, but I can open an issue about it as well

This was referenced Sep 11, 2020
Merged
Open
@acomodi
Copy link
Collaborator Author

acomodi commented Sep 11, 2020

Opened issues:

@vaughnbetz vaughnbetz merged commit 95843c2 into verilog-to-routing:master Sep 11, 2020
@vaughnbetz
Copy link
Contributor

Thanks @acomodi. This is a major PR !
Tracking the remaining nits / cleanups with other issues sounds good.
Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@litghost litghost litghost approved these changes

@HackerFoo HackerFoo Awaiting requested review from HackerFoo

@vaughnbetz vaughnbetz Awaiting requested review from vaughnbetz

Assignees
No one assigned
Labels
build Build system lang-cpp C/C++ code lang-make CMake/Make code libarchfpga Library for handling FPGA Architecture descriptions libvtrutil tests VPR VPR FPGA Placement & Routing Tool VTR Flow VTR Design Flow (scripts/benchmarks/architectures)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@acomodi @vaughnbetz @litghost @HackerFoo @symbiflow-robot