Allow router to report successful routing even if RCV fails to resolve hold #1540

dpbaines · 2020-09-13T23:02:17Z

Description

Makes it so that RCV will report a successful routing regardless of whether hold has been resolved. This is just a simple check that allows it to report a successful routing if routing is feasible, and the maximum iteration count is reached. This doesn't fix the underlying problem of why RCV fails to resolve hold for some circuits (LU8PEEng and LU32PEEng) but it's a quick patch.

I want to figure out the underlying issue and also address that in this PR so I don't want to merge quite yet.

Related Issue

Closes #1533

Motivation and Context

Make RCV more robust and versatile on multiple circuits.

How Has This Been Tested?

I tested this on LU32PEEng and LU8PEEng circuits with harsh hold constraints and RCV enabled, and verified that it reports a successful routing even though hold isn't resolved. I don't see a point in running the full qor tests as this isn't a possibly breaking change yet.

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

dpbaines · 2020-09-13T23:03:11Z

I don't want to merge this yet, as I'm hoping to get around to figuring out why the LU circuits are failing, and fix those.

mithro · 2020-09-14T13:17:21Z

@dpbaines -- Any chance we could fail by default (and instead add a flag which allows things to report success even if there are hold violations)? Designs with hold violations are broken. Reporting that PnR is successful when the design is broken violates the contract that a successful exit code means a successfully place and routed design which will work.

vaughnbetz · 2020-09-14T15:27:13Z

@mithro I think we should report routing success in this case; the hold violation will still be reported, so reporting routing success does not imply timing success.

This matches what commercial tools do -- Quartus will report a successful routing but failed hold (or setup) timing constraints if any failed constraints are present. You can still analyze routability, look at the routing, get a timing report etc. Simply stopping makes it harder to look at the output (don't get a full analysis), and precludes programming a board (useful as hold margins usually have guardband, so you could still test to see if it works on a board. If the failure is small, it usually will work).

vaughnbetz · 2020-09-14T15:30:52Z

vpr/src/route/route_timing.cpp

@@ -1987,9 +1987,11 @@ bool is_iteration_complete(bool routing_is_feasible, const t_router_opts& router
    //in addition to routing being legal and the correct budgeting algorithm being set.

    if (routing_is_feasible) {


I would re-order this if
Test the non-RCV (routing_budgets_algorithm != YOYO) first, so someone who is not thinking of RCV has less to read. Then the rest of the code.

vaughnbetz · 2020-09-14T15:31:24Z

vpr/src/route/route_timing.cpp

        if (router_opts.routing_budgets_algorithm != YOYO) {
            return true;
-        } else if (router_opts.routing_budgets_algorithm == YOYO && (timing_info->hold_worst_negative_slack() == 0 || rcv_finished) && itry != 1) {
+        } else if (router_opts.routing_budgets_algorithm == YOYO && (timing_info->hold_worst_negative_slack() == 0 || can_router_finish) && itry != 1) {


Shouldn't the test for hold_worst_negative_slack be >= 0? Seems a lot safer than a test for precisely 0.

I would agree, but worst_negative_slack is equal to zero if worst hold slack is positive. Therefore they would equate to the same thing, but I could change it in case that changes in the future.

OK, lets do >= for safety and readability though.

vaughnbetz · 2020-09-14T15:46:06Z

I think a cleaner fix is possible.

is_iteration_complete is not a very clear name for this function. The header file also doesn't have a function description. A better name would be consider_routing_legal ()
A larger restructuring (maybe more than you want to do right now) would be:

replace the call to is_iteration_complete with a call to is_feasible_routing
make sure the is_better_quality_routing routine checks for hold and store the routing that has the best timing (first priority = hold if hold slack < 0, second priority = setup slack / current code).
if you can resolve more hold violations with more iteration, then you'd restructure some later code to also check a new hold_not_legal variable (not just how many convergences there are).

I think this would be cleaner and more powerful, but it would take more testing.

I think code that says the routing is legal, even if Th is not resolved in the end, should go in no matter what. No algorithm is perfect and sometimes hold violations are not resolvable without running out of wiring. So even if you can resolve LU8 and LU32's hold problems, code to say the routing is legal should be added.

dpbaines · 2020-09-14T16:44:48Z

@vaughnbetz Your suggestions make sense, I'll try to take a better look at them this week at some point.

…s if RCV is enabled, with others

dpbaines · 2020-09-20T05:28:52Z

@vaughnbetz I've implemented your suggestions so far. I've eliminated the is_iteration_complete function, and instead modified the is_better_quality_routing function. If RCV is enabled the is_better_quality_routing function will prioritize routing with improved hold slack, and then setup slack. Otherwise it uses the old code path.

It will also indicate that routing was successful, however it won't allow the router to break early unless either hold slack >= 0, or RCV detects it can finish early, as exists in the current code. I haven't had a lot of time to figure out why LU8 and LU32 struggle to resolve, so I wanna keep picking away at it.

Let me know what you think of the current code. Or if you think is_better_quality should always prioritize hold even in the regular router (which would clean up code).

I have tested this on a few key circuits and it seems to be working well. For LU8 and LU32 it returns the routing that had the lowest negative hold slack.

vaughnbetz · 2020-09-22T00:01:34Z

vpr/src/route/route_timing.cpp

@@ -2007,35 +2006,66 @@ bool should_setup_lower_bound_connection_delays(int itry, const t_router_opts& /
 static bool is_better_quality_routing(const vtr::vector<ClusterNetId, t_traceback>& best_routing,
                                      const RoutingMetrics& best_routing_metrics,
                                      const WirelengthInfo& wirelength_info,
-                                      std::shared_ptr<const SetupHoldTimingInfo> timing_info) {
+                                      std::shared_ptr<const SetupHoldTimingInfo> timing_info,
+                                      const t_router_opts& router_opts) {
    if (best_routing.empty()) {
        return true; //First legal routing
    }

    //Rank first based on sWNS, followed by other timing metrics


This comment doesn't seem to match the code below.

vaughnbetz · 2020-09-22T00:02:01Z

vpr/src/route/route_timing.cpp

-        } else if (timing_info->setup_total_negative_slack() < best_routing_metrics.sTNS) {
-            return false;
-        }
+            if (timing_info->hold_worst_negative_slack() > best_routing_metrics.hWNS) {


This is the RCV off case? If so, why is it making decisions based on hold?

I guess that check was there as a secondary quality check if the setup slacks were tied, to allow the router to pick routing with improved hold. I get that this would be unlikely though, so I guess it makes sense to remove this check for non-RCV routing.

vaughnbetz · 2020-09-22T00:02:37Z

vpr/src/route/route_timing.cpp

-        }
+            if (timing_info->hold_worst_negative_slack() > best_routing_metrics.hWNS) {
+                return true;
+            } else if (timing_info->hold_worst_negative_slack() > best_routing_metrics.hWNS) {


Both the if and else if condition seem to be the same? Copy and paste error?

vaughnbetz · 2020-09-22T00:03:06Z

vpr/src/route/route_timing.cpp

-        } else if (timing_info->hold_worst_negative_slack() > best_routing_metrics.hWNS) {
-            return false;
-        }
+            if (timing_info->hold_total_negative_slack() > best_routing_metrics.hTNS) {


Needs a comment as this looks like a repeat of the prior 5 lines of code until you look very closely (total negative slack instead of worst negative slack).

vaughnbetz · 2020-09-22T00:06:22Z

vpr/src/route/route_timing.cpp

-            return true;
-        } else if (timing_info->hold_total_negative_slack() > best_routing_metrics.hTNS) {
-            return false;
+            if (timing_info->setup_worst_negative_slack() > best_routing_metrics.sWNS) {


You could reduce code duplication and make the code more readable by making helper functions: setup_comparison and hold_comparison that encapsulate the (1) test worst case slack first and then check total negative slack, and return better, worse or tied.

Those routines would also give you a place to comment that we don't care about positive slack (consider it a tie) and only check to see if negative slacks have improved.

vaughnbetz · 2020-09-22T00:09:06Z

vpr/src/route/route_timing.cpp

-            || early_reconvergence_exit_heuristic(router_opts, itry_since_last_convergence, timing_info, best_routing_metrics)) {
+        if ((legal_convergence_count >= router_opts.max_convergence_count
+             || router_iteration_stats.connections_routed == 0
+             || early_reconvergence_exit_heuristic(router_opts, itry_since_last_convergence, timing_info, best_routing_metrics))


Could move hold_legal into the early_reconvergence_exit_heuristic routine, which I think would be easier to read by simplifying this condition and giving you a place to comment why you're not saying you're exiting early.

I agree that would be easier to read, but because the default max_convergence_count is 1, the router would stop early regardless. What I could do is to move all of these conditions into a seperate function?

Still outstanding

When RCV is selected you could change the legal_convergence_count default to some larger number (we can have dependent option defaults; take a look at the option parsing code and you should find an example). Then if the early_reconvergence_exit_heuristic understands hold and rcv it can do the right thing. The if statement won't look as long.

You could also move the whole if into a control routine; that would also work. But I think changing legal_convergence_count makes sense no matter what, as I believe you've found RCV wants a higher value for this setting. Better to do that by actually changing the value than writing if statements to work around it.

vaughnbetz · 2020-09-22T00:13:24Z

Thanks David. I have some questions and suggestions above. One is major: I think the code is backwards, and others are useful for readability.

… and more comments

…vtr-verilog-to-routing into rcv_fail_fix

vaughnbetz · 2020-09-22T13:42:38Z

vpr/src/route/route_timing.cpp

@@ -579,11 +588,17 @@ bool try_timing_driven_route_tmpl(const t_router_opts& router_opts,
            pres_fac = update_pres_fac(router_opts.initial_pres_fac);
        }

+        if (budgeting_inf.if_set()) {
+            // If total negative slack is greater than or equal to 0, or RCV is finished, allow router to complete
+            hold_legal = timing_info->hold_total_negative_slack() >= 0 || rcv_finished_count == 0;


If this variable doesn't really mean hold is legal (it seems to mean RCV should stop trying) then you should give it a different name.

vaughnbetz · 2020-09-22T13:45:15Z

vpr/src/route/route_timing.cpp

-        } else if (timing_info->hold_worst_negative_slack() > best_routing_metrics.hWNS) {
-            return false;
-        }
+            if (hold_comparison > 0) {


This code could be shortened by checking if YOYO is enabled, and if so, checking hold and returning whatever is better, or doing nothing in a tie. Then check setup (no matter what), then wirelength.

You're right, I should have seen this! Will fix thanks.

vaughnbetz · 2020-09-22T13:46:18Z

The compare_setup and compare_hold routines have cleaned up some code. I left some more comments on ways to shorten / clarify further.

dpbaines added 2 commits September 13, 2020 18:08

Patching RCV fail criteria, doesn't fail if routing is feasible

16bc19e

Move check to the is iteration complete function

7f00300

probot-autolabeler bot added lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool labels Sep 13, 2020

vaughnbetz reviewed Sep 14, 2020

View reviewed changes

vaughnbetz requested changes Sep 14, 2020

View reviewed changes

Refactored router code to store routing with best hold characteristic…

e62a48b

…s if RCV is enabled, with others

Make format

2ef1474

vaughnbetz requested changes Sep 22, 2020

View reviewed changes

dpbaines added 3 commits September 21, 2020 21:23

Changes to is_better_routing function in route_timing, easier to read…

fc0fdfc

… and more comments

Merge branch 'rcv_fail_fix' of https://github.com/verilog-to-routing/…

c45b4a7

…vtr-verilog-to-routing into rcv_fail_fix

Make format

d9babb0

vaughnbetz reviewed Sep 22, 2020

View reviewed changes

		@@ -1987,9 +1987,11 @@ bool is_iteration_complete(bool routing_is_feasible, const t_router_opts& router
		//in addition to routing being legal and the correct budgeting algorithm being set.

		if (routing_is_feasible) {

Allow router to report successful routing even if RCV fails to resolve hold #1540

Are you sure you want to change the base?

Allow router to report successful routing even if RCV fails to resolve hold #1540

Uh oh!

Conversation

dpbaines commented Sep 13, 2020

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

dpbaines commented Sep 13, 2020

Uh oh!

mithro commented Sep 14, 2020

Uh oh!

vaughnbetz commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 14, 2020

Uh oh!

dpbaines commented Sep 14, 2020

Uh oh!

dpbaines commented Sep 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 22, 2020

Uh oh!

Uh oh!