Merging RCV branch into master #1395

dpbaines · 2020-07-03T20:34:04Z

Description

This PR is a work in progress and still has a significant overhead on top of the regular VPR flow

RCV is an algorithm which allows for hold time violations to be resolved by allowing the router to create longer connections where necessary
Paper with more info here: https://www.eecg.utoronto.ca/~vaughn/papers/tcad2008_rcv.pdf
Work done by: Ming Yang Kun, Richard Ren and David Baines

Related Issue

Motivation and Context

How Has This Been Tested?

I tested this in a variety of scenarios on both the titan benchmark suite, as well as qor reg chain. Results of the data are located here:

UPDATED TITAN: https://docs.google.com/spreadsheets/d/10omM-uNtj-8o-3DnEfGh7C6fznX8va63Y85yTmocNqE/edit?usp=sharing
UPDATED QOR CHAIN: https://docs.google.com/spreadsheets/d/1pJdYBgbMMMudm0-0STI7htba0uHOeQ7VXBu1NcMzAME/edit?usp=sharing

As well I analyzed the results of which in a document here:
https://docs.google.com/document/d/1mZoHqO3H5mYvD44dt2-Ai-eIc4rlrHm4dhSMOlLKmqo/edit?usp=sharing

In summary, when RCV is turned off there should be minimal runtime impact due to the extra if statements in the core router code. I've tried to minimize this in a number of ways.

When RCV is turned on, there is a more significant impact on runtime, which is mostly caused by the route budgeting. However even in a lot of beyond worst case hold cases, it still is able to resolve intercluster hold violations mostly. There is still work to be done for larger circuits, as well as possibly dealing with intra cluster hold violations.

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

…ing much better performance

…ead of reference

…ll as extra debugging helps

vaughnbetz · 2020-09-08T02:54:07Z

Thanks David.
Can you attach Th achieved results for the degraded architcture (VTR, 500 ps Th) with and without RCV? I see the cost of RCV, but not the benefit in the data so far since no Th is included.
QoR failures are all on bidir architectures (old), and are on circuit that got much faster in strong and one that got significantly slower in nightly. Seems OK.
The Titan and vtr qor results look OK so far. RCV is failing on larger circuits, so there is probably a problem in the algorithm somewhere but we can tune that after this PR.

vaughnbetz · 2020-09-08T02:56:47Z

vpr/src/route/route_path_manager.h

+/* Extra path data needed by RCV, seperated from t_heap struct for performance reasons
+ * Can be accessed by a pointer, won't be initialized unless needed
+ *
+ * path_rr: The entire partial path up until the route tree


Explain what this is. Entire partial path until we reach either the net SOURCE or a part of the routing tree that has already been created for other connections for this net.

vaughnbetz · 2020-09-08T02:59:30Z

vpr/src/base/read_options.cpp

@@ -2009,7 +2013,7 @@ argparse::ArgumentParser create_arg_parser(std::string prog_name, t_options& arg
            " * criticality: Sets the minimum budgets to 0 and the maximum budgets as a function of delay and criticality (net delay/ pin criticality) [EXPERIMENTAL]\n"


This help on options doesn't seem to match the option values below. Can you update it?

Sure thing, a simple next PR could be to give RCV it's own distinct option in the router, but for now I'll update this.

vaughnbetz · 2020-09-08T02:59:58Z

vpr/src/base/vpr_types.h

@@ -1017,6 +1017,7 @@ enum e_routing_failure_predictor {
 };
 enum e_routing_budgets_algorithm {


Add comment here on what this controls. Either describe the options or refer back to command option help.

vaughnbetz · 2020-09-08T03:05:39Z

vpr/src/route/route_path_manager.h

+    PathManager();
+    ~PathManager();
+
+    // Checks if the target node exists in the route tree, or current backwards path variable


Do you have a comment that explains how RCV can create loops, and that two tests are necessary to prevent loops (check the partial route created for other connections on the net (route_tree_set) and check the prior nodes on the path that is being expanded/explored for a given heap element (partial path) for the current connection. This latter part needs to be stored for every partial path (heap element).

If you don't have such an overview comment, you should add it, probably near the top of route_path_manager.h.

vaughnbetz · 2020-09-08T03:07:42Z

Did a quick review of the code, which generally looks good. I added a few requests for comments / command line help above.
If you don't have high-level comment blocks on (1) why we need a path manager and how it works (mentioned above) and (2) how RCV works, you should add them. If they exist already, can you point me at them?

dpbaines · 2020-09-08T03:32:13Z

@vaughnbetz Hi, as per the Thold values for RCV on and off, they are in the spreadsheet under the Formatted Raw Data section. I included both total hold and worst case hold slacks for each test. Sorry that wasn't noted somewhere!

As per the failures on larger circuits, I can look into whats causing them for a future pull request. Thanks for looking over this, I'll make the changes you suggested right now.

…g/vtr-verilog-to-routing into router_rcv_2.0

vaughnbetz · 2020-09-08T12:58:35Z

vpr/src/base/vpr_types.h

-    MINIMAX,
-    YOYO,
+    MINIMAX,          // Use MINIMAX-PERT algorithm to allocate budgets
+    YOYO,             // Use MINIMAX as above, and enable RCV algorithm to resolve negative hold slack
    SCALE_DELAY,
    DISABLE


Should comment DISABLE too as it's an important one: currently turns off RCV and delay budgets and uses default router.

vaughnbetz · 2020-09-08T12:59:24Z

vpr/src/route/route_path_manager.h

+ * Having these in t_heap creates significant performance issues when RCV is disabled
+ * A t_heap_path pointer is instead stored in t_heap, which is selectively allocated only when RCV is enabled
+ * 
+ * If the _is_enabled flag is true, alloc_path_struct allocates t_heap_path structures, otherwise will be a NOOP */


Very nice comment -- thanks!

vaughnbetz

Thanks, new comments look good. One more request embedded above.
Do you know if the circuits that fail with RCV run out of memory or unroute? Good to describe that here, as well as the experiment conditions (e.g. I think the vtr circuits channel width was 300 or 400 with RCV, but am not sure of the value).

dpbaines · 2020-09-08T15:54:53Z

@vaughnbetz For the QOR reg tests, the two circuits that failed for RCV ON were LU8PEEng and LU32PEEng, which didn't have a memory issue. They failed because there was still negative hold slack, and RCV didn't detect that this was probably being caused by global/ignored nets. This is a tunable fix, because I do have a function that is supposed to detect this.

For the Titan tests the following circuits are reported as failed for the following reasons:

Gaussian blur: This failed because I stopped it early as it was taking all weekend, I did keep it running and it actually passed. I can update the doc to include this.

LU_Network: This failed the following assertion:

## Initializing router criticalities took 21.20 seconds (max_rss 9526.5 MiB, delta_rss +90.6 MiB)
---- ------ ------- ---- ------- ------- ------- ----------------- --------------- -------- ---------- ---------- ---------- ---------- --------
Iter   Time    pres  BBs    Heap  Re-Rtd  Re-Rtd Overused RR Nodes      Wirelength      CPD       sTNS       sWNS       hTNS       hWNS Est Succ
      (sec)     fac Updt    push    Nets   Conns                                       (ns)       (ns)       (ns)       (ns)       (ns)     Iter
---- ------ ------- ---- ------- ------- ------- ----------------- --------------- -------- ---------- ---------- ---------- ---------- --------
Warning 16817: 62356 timing endpoints were not constrained during timing analysis
   1   80.6     0.0    0 6.1e+08  404083  741539  432510 ( 2.904%) 5647923 (26.7%)    7.316 -2.113e+06     -7.316      0.000      0.000      N/A
/home/bainesda/Documents/vtr-verilog-to-routing/vpr/src/timing/PostClusterDelayCalculator.tpp:352 atom_net_delay: Assertion !std::isnan(edge_delay.value())' failed.
Command terminated by signal 6

I haven't encountered this before. This appears to fail during the route budgeting stage with a weird call to tatum. This seems like it's probably an easy fix... I will look into this right now

By the way I'm going to update the golden results for strong_bidir tests.

…g/vtr-verilog-to-routing into router_rcv_2.0

vaughnbetz · 2020-09-09T01:39:21Z

Thanks David.

For RCV on, degraded Th, there are four circuits with no results in the clean comparison to maser. But 2 of those (mcml and stereovision0) seem to have completed (but failed hold). Is this just a parsing issue? If it's easy to fill in the missing numbers it would be good to add them in the tables and the averages.
RCV on with degraded hold doesn't resolve hold on 4 circuits (LU8, LU32, mcml and stereovision0). I don't want to try to tune this further at this point, but if you have any idea of why let me know (and add it to your overall document) as it provides a starting point for future work.
Why do LU8 and LU32 not produce full output when Th isn't resolved? It would be better for them to still produce a final routing and statistics if they obtain a legal routing.
LU_Network error: If you have time to look at that, I'd certainly appreciate it. If not, filing an issue would still be helpful. Running the sanitizer build on RCV would be a good idea if you haven't done so; maybe there's a memory fault.

vaughnbetz · 2020-09-09T01:42:30Z

This looks ready to merge to me. I have some qor questions above, but none seem to be merge gating.

dpbaines · 2020-09-09T02:18:49Z

@vaughnbetz To address your 4 concerns:

My bad I copied the document and it didn't auto complete MCML and stereovision circuits, I have filled these numbers in just now.
I made a little comment on the spreadsheet as to why this happened, but adding it to the document is also a good idea! The issues for (MCML and stereovision0) stem from global/ignored nets with hold violations. RCV right now will tell the router to complete if it's detecting that only global/ignored nets have hold issues. These are unresolvable right now, but will still be reported in the hTNS and return a successful routing. This may be the incorrect way to approach it, in which case would be a good idea for a future PR.
As for the other 2 failed circuits (LU8 and LU32), I need to do a bit more investigation. Right now if RCV is active, and the router reaches the maximum iteration count with a none zero hTNS it reports a fail. In which case we don't get a final report. This feature stops the router from finishing before RCV can resolve hold completely, but can result in these failed routings. This can be improved by making RCV smarter at detecting when it's finished, and when it can give up. I'm not sure why it struggles to resolve hold however. This is something I want to look into for the next PR too, or at the very least mention in the document.
It doesn't appear to be due to a memory fault. It's a failed assertion check in tatum, which I think can be traced back to the perform_STA function in route_budgets.cpp. I'll dig into it however it will take some time as it is an outlier case, so it can be work for a future PR. I'll try and get it sorted soon.

vaughnbetz · 2020-09-09T03:21:39Z

Thanks David. 3 is probably resolvable: basically if the routing is legal we should report success, whether Th is met or not. It's fine to have RCV keep trying to resolve hold, but the final condition of whether this routing is legal or not should understand it is legal and just move on to reporting. Hopefully the code for that is simple / clean.

I agree 4 sounds more involved; sounds like something that can wait.

vaughnbetz · 2020-09-09T03:22:27Z

Should be clear: 3 and 4 should both be left to future PRs. I just think 3 is a lot easier than 4.

dpbaines · 2020-09-09T03:30:03Z

@vaughnbetz thanks for getting back to me, sounds good I'll note these two issues in the document if I don't get around to fixing them soon. I might move to patch 3 right after this.

dpbaines · 2020-09-09T21:04:56Z

I updated the golden results for the vtr_reg_nightly/vtr_bidir regression test because it was performing differently. Let me know if you if you disagree with this

vaughnbetz · 2020-09-09T23:22:16Z

Updating that QoR test looks OK to me.

vaughnbetz · 2020-09-10T18:49:50Z

Read the docs failed but I can't see how that can possibly be related as no documentation files were changed in this PR. Keith confirmed in the meeting today that he's happy with the new code, as am I. Nice work pulling this together David!

David Baines and others added 16 commits June 4, 2020 15:35

Merged the older rcv code into the up to date master

69f3952

Fixing crashes and merging issues for rcv algorithm

716c6e6

Merge branch 'master' into router_rcv_2.0

70f6376

Bug fixes to RCV code in connection_router and route_timing

0ef8fce

Adding support for a quadratic penalty for routing costs

1cadf6b

Merge branch 'master' into router_rcv_2.0

d988892

Merge branch 'master' into router_rcv_2.0

8694375

Performance enhancements to added RCV data structure improvements

d298dd1

Changed how loops are detected for RCV, performance improvements

cd117f5

Merge branch 'master' into router_rcv_2.0

2cd43c2

Moved t_heap vectors into different struct for performance

aef7a7c

Merge branch 'master' into router_rcv_2.0

f1b7afd

Changed route tree nodes set to a pointer instead of a reference, giv…

609dc8e

…ing much better performance

Merge branch 'master' into router_rcv_2.0

f5a180c

Fixing route diag router code

eb267e4

Spelling mistake in main.cpp

dce6af6

probot-autolabeler bot added lang-cpp C/C++ code tests VPR VPR FPGA Placement & Routing Tool VTR Flow VTR Design Flow (scripts/benchmarks/architectures) labels Jul 3, 2020

dpbaines added 10 commits July 6, 2020 21:51

Fixed show setup string format from int to float

23c0d10

Refactored connection router to pass route tree nodes as pointer inst…

e36a0bb

…ead of reference

Fixing RCV code and adding congestion and delay map lookahead functions

30dd55e

Tuning RCV so it can now resolve even extreme hold time violations

7896aa3

Memory issues fixes

ee81ad3

Memory leak fix

4057cbd

Fixes to map lookahead and other attempted performance improvements

093c9a1

Merge branch 'master' into router_rcv_2.0

5c8d3bf

Performance enhancements by altering the cost function for RCV, as we…

fd3dbe0

…ll as extra debugging helps

Changes to route budgets to improve budget allocation

b702725

vaughnbetz reviewed Sep 8, 2020

View reviewed changes

dpbaines added 2 commits September 8, 2020 00:32

Commenting and command help fixes

91e1e5a

Merge branch 'router_rcv_2.0' of https://github.com/verilog-to-routin…

fefa645

…g/vtr-verilog-to-routing into router_rcv_2.0

vaughnbetz reviewed Sep 8, 2020

View reviewed changes

vaughnbetz approved these changes Sep 8, 2020

View reviewed changes

dpbaines added 2 commits September 8, 2020 11:40

Commenting fixes

0b26ced

Make format

eed4296

dpbaines added 3 commits September 8, 2020 12:08

Update golden results for stong_bidir

538c7f3

Merge branch 'router_rcv_2.0' of https://github.com/verilog-to-routin…

3fb14e1

…g/vtr-verilog-to-routing into router_rcv_2.0

Slack crit debug text is back

2901f19

Updating vtr_bidir golden_results

aaec4fa

vaughnbetz merged commit 7ae49bf into master Sep 10, 2020

vaughnbetz deleted the router_rcv_2.0 branch September 10, 2020 18:50

		@@ -2009,7 +2013,7 @@ argparse::ArgumentParser create_arg_parser(std::string prog_name, t_options& arg
		" * criticality: Sets the minimum budgets to 0 and the maximum budgets as a function of delay and criticality (net delay/ pin criticality) [EXPERIMENTAL]\n"

		@@ -1017,6 +1017,7 @@ enum e_routing_failure_predictor {
		};
		enum e_routing_budgets_algorithm {

Merging RCV branch into master #1395

Merging RCV branch into master #1395

Uh oh!

Conversation

dpbaines commented Jul 3, 2020 • 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!

vaughnbetz commented Sep 8, 2020

Uh oh!

vaughnbetz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dpbaines Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 8, 2020

Uh oh!

dpbaines commented Sep 8, 2020

Uh oh!

vaughnbetz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

dpbaines commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaughnbetz commented Sep 9, 2020

Uh oh!

vaughnbetz commented Sep 9, 2020

Uh oh!

dpbaines commented Sep 9, 2020

Uh oh!

vaughnbetz commented Sep 9, 2020

Uh oh!

vaughnbetz commented Sep 9, 2020

Uh oh!

dpbaines commented Sep 9, 2020

Uh oh!

dpbaines commented Sep 9, 2020

Uh oh!

vaughnbetz commented Sep 9, 2020

Uh oh!

vaughnbetz commented Sep 10, 2020

Uh oh!

Uh oh!

dpbaines commented Jul 3, 2020 •

edited

Loading

dpbaines commented Sep 8, 2020 •

edited

Loading