Routing predictor degenerate fit - #3795
Conversation
estimate_success_iteration() fits a line to log(overuse) and extrapolates it, but discards everything about the fit except the extrapolated iteration. Keep a summary of the fit (slope, intercept, window and sample count) so callers can tell a model which could not extrapolate from one predicting slow convergence, and can report which data an abort decision was based on.
estimate_success_iteration() was called before add_iteration_overuse(), so the prediction reported for an iteration was fit without that iteration's overuse, lagging the data by one iteration. It also left the cached slope and the fit behind the estimate computed over different windows. Estimate after recording.
When overuse is flat or rising the fitted slope is non-negative and the model cannot extrapolate, which estimate_success_iteration() reports as infinity. That infinity then compares greater than any finite abort threshold, so the router gives up hardest exactly when its model is least informative. Overuse commonly plateaus for a few iterations before it falls, so in SAFE mode wait a bounded number of iterations for a usable estimate. The grace applies only until the model first extrapolates; a later infinity means overuse is climbing again and is acted on immediately. AGGRESSIVE mode is unchanged.
An infinite or very large estimated success iteration gave no indication of which data produced it, making a premature abort hard to diagnose. Report the fit window, sample count and slope alongside the estimate: per iteration under --route_verbosity > 1, and unconditionally in the abort message.
github-actions
Bot
added
VPR
|
This PR also addresses issue #3794 |
…ference simple_linear_regression() took its input vectors by value, copying them on every call. Now that fit_model() reads hist_iters after the call the copy is no longer elided, and GCC 13 at -O3 reports a false-positive -Wnull-dereference inside the vector copy constructor, which fails the warnings-as-errors build. The function only reads its inputs, so take them by const reference; this also drops two allocations per fit.
AlexandreSinger
left a comment
AlexandreSinger
left a comment
There was a problem hiding this comment.
Thanks @amin1377 .
Overall I like what you are trying to do, I just had some comments about keeping the code more isolated into the RoutingPredictor class.
Could you also please run and compare the MCNC and VTR_CHAIN NightlyTests. As far as I am aware, those are the only weekly tests that we care about that do min channel width sweeps. I do not think this will have huge changes, but its good to be paranoid.
| " estimated success iteration %.1f, abort threshold %.1f%s\n", | ||
| predictor_fit.first_iteration, predictor_fit.last_iteration, predictor_fit.num_samples, | ||
| predictor_fit.slope, est_success_iteration, abort_iteration_threshold, | ||
| awaiting_usable_prediction ? " (waiting for an extrapolable fit)" : ""); |
There was a problem hiding this comment.
I think the code added to route.cpp can be made a lot cleaner.
You have added a bunch of new variables and specific code to this very long function. I think it would be better to hide this in the routing_predictor object.
One idea is the following:
RoutingPredictor routing_predictor(...); // Store and init initial_degenerate_predictions and predictor_has_extrapolated internally.
// ...
float est_success_iteration = routing_predictor.estimate_success_iteration();
// ...
if (routing_predictor.prediction_is_valid(overuse_info.overused_nodes)) {
if (!std::isnan(est_success_iteration) && est_success_iteration > abort_iteration_threshold ...) {
VTR_LOG("Routing aborted...");
break; // Abort
}
}The prediction_is_valid method would then contain the comparison with ROUTING_PREDICTOR_MIN_ABSOLUTE_OVERUSE_THRESHOLD, the awaiting usable prediction, and the log message that you have added (which I strongly feel should be hidden within the class.
I think this will make this code more maintainable. What do you think?
| } else { | ||
| predictor_has_extrapolated = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This incrementing of initial_degenerate_predictions does not make sense to me. Suppose we get a non-infinite success iteration early. predictor_has_extrapolated would then be true, but the initial_degenerate_predictions would be stuck at a low number. It would then never increment high enough to turn awaiting_usable_prediction off, since predictor_has_extrapolated is now set to true.
I think I see what you wanted to do here, if the est_success_iteration is non-infinite at any point we do not need to do the increment; however, I think this needs to be fixed to do that correctly.
There was a problem hiding this comment.
I’m not sure I’m following your comment here. My goal isn’t to wait a few iterations every time the estimator returns inf, since that could significantly increase runtime. I only want to allow that grace period immediately after the fitter’s ramp-up period.
|
|
||
| // If overuse is flat or increasing, the predictor cannot extrapolate and returns infinity. | ||
| // In SAFE mode, allow a few such predictions before giving up. | ||
| constexpr size_t ROUTING_PREDICTOR_MAX_DEGENERATE_ITERATIONS = 10; |
There was a problem hiding this comment.
I think this comment can be improved. From my understanding, this variable sets "how many times can the estimated prediction be infinity (not NaN) before we give up". I misunderstood this originally to be how many iterations of CONTINUOUS infinities; which would make 10 way too large in my opinion.
There was a problem hiding this comment.
It means how many consecutive iterations after min_history the slope can remain positive (i.e., the estimated iteration is inf) before giving up.
Address review feedback: hide the degenerate-fit tracking, the minimum overuse threshold check, and the verbose fit logging inside a new RoutingPredictor::prediction_is_valid() method instead of open-coding them in route(). The predictor now takes safe_mode and verbosity at construction and caches its last estimate. No behavior change.
Move all predictor state updates (model fit, success-iteration estimate, degenerate-fit tracking, verbose logging) into add_iteration_overuse(), the natural once-per-iteration entry point. This removes the implicit call-order contract between estimate_success_iteration() and prediction_is_valid(), which are now const queries of cached state. Also assert that iterations are recorded once each in increasing order, and drop a redundant second regression per routing iteration.
…er out-param The fit summary's slope and y-intercept fully determine the fitted linear model, so fit_model() can return the summary by value and let callers reconstruct the model from it, removing the optional pointer parameter.
fit_model's callers are all RoutingPredictor members passing the same member vectors, so make it a private const method that reads them directly, leaving only the history factor as a parameter. Wrap the remaining file-local math helpers (LinearModel, variance, covariance, simple_linear_regression) in an anonymous namespace, fixing their accidental external linkage, and drop the now-unneeded forward declarations.
vaughnbetz
left a comment
vaughnbetz
left a comment
There was a problem hiding this comment.
I second Alex's comment on keeping code out of the main router routine (which is too long already) as much as we can.
A higher level question: we already have a minimum number of iterations that have to occur before we start predicting if routing will converge. How does that interact with this new feature? Do we have designs that (after the first iterations) temporarily show an increasing trend of routing overuse, but ultimately converge? Can we cover those cases by tuning the initial iterations where we don't check the prediction and the window over which we fit an extrapolation? Or do we need this new feature? If tuning the existing features solves the issue that is simper.
| LinearModel simple_linear_regression(std::vector<size_t> x_values, std::vector<float> y_values); | ||
| LinearModel fit_model(const std::vector<size_t>& iterations, const std::vector<size_t>& overuse, float history_factor); | ||
| LinearModel simple_linear_regression(const std::vector<size_t>& x_values, const std::vector<float>& y_values); | ||
| LinearModel fit_model(const std::vector<size_t>& iterations, |
There was a problem hiding this comment.
Shouldn't this be static to indicate it is local to the file?
Also needs doxygen.
| //Estimate at what iteration we will converge to a legal routing | ||
| if (overuse_info.overused_nodes > ROUTING_PREDICTOR_MIN_ABSOLUTE_OVERUSE_THRESHOLD) { | ||
| //Only consider aborting if we have a significant number of overused resources | ||
| if (routing_predictor.prediction_is_valid() |
There was a problem hiding this comment.
This is adding less code to a long routine than the original PR (as I recall), but see if you can further encapsulate the logic so we can just ask if we should give up on routing to the routing predictor, and have it do all the logic and print any messages.
Maybe some of the other (existing) router prediction logic could also be abstracted in that way.
If you can't figure out a way to do it, I will live with it, but this routine is too long so the more logic we can move out of it the better.
3 participants
The routing predictor estimates convergence by fitting a line to recent
log(overuse)values. When overuse is flat or increasing, the model cannot extrapolate and returns infinity, which can cause SAFE mode to abort too early even if routing would later converge. This PR adds a bounded grace period for these early non-extrapolable estimates, fixes the estimate to include the current iteration’s overuse, and improves logging of the predictor fit. AGGRESSIVE and OFF behavior is unchanged.