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.

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

Remove budget based PNR #1149

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Remove budget based PNR #1149

merged 1 commit into from
Jun 20, 2023

Conversation

rowanG077
Copy link
Contributor

@rowanG077 rowanG077 commented May 11, 2023

TODO:

  • Test on real world design to see if everything still functions as expected

@@ -51,7 +51,7 @@ wirelen_t get_net_metric(const Context *ctx, const NetInfo *net, MetricType type
continue;
if (timing_driven) {
delay_t net_delay = ctx->predictArcDelay(net, load);
auto slack = load.budget - net_delay;
auto slack = -net_delay;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gatecat This is one of the only places I had to remove budget where I'm not sure I need to replace it with something else.

Copy link
Member

@gatecat gatecat May 11, 2023

Choose a reason for hiding this comment

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

Looks good - this is rarely if ever used code these days anyway (and quite possibly ripe for a cleanup - but don't worry about that as part of this work stream)

test.v Outdated Show resolved Hide resolved
@gatecat
Copy link
Member

gatecat commented May 19, 2023

Friendly ping, have you had another chance to look at this?

@rowanG077
Copy link
Contributor Author

Yes! I will update after the weekend.

@rowanG077
Copy link
Contributor Author

rowanG077 commented May 25, 2023

It now prints out the critical path using only the new API. I have compared the outputs of a real world design on the ECP5. ~60% utilisation with 5 clocks and the reports match one to one. What is missing is that async paths aren't printed yet.

I want to separate out the code that prints critical paths since it's quite large and makes it harder to understand what code is just for the analysis and what part is just for printing. If you are oke with this I want to move all of that to a timing_reporter.cc file.

common/kernel/timing.cc Outdated Show resolved Hide resolved
shell.nix Outdated Show resolved Hide resolved
@gatecat
Copy link
Member

gatecat commented May 26, 2023

Some other comments:

  • has your testing covered designs with a critical path from posedge to negedge or vice versa?
  • there's a CI failure as we have -Wall there: error: ‘const char* nextpnr_generic::{anonymous}::edge_name(nextpnr_generic::ClockEdge)’ defined but not used [-Werror=unused-function] 33 | const char *edge_name(ClockEdge edge) { return (edge == FALLING_EDGE) ? "negedge" : "posedge"; }
  • there's another CI failure because himbaechel needs getBudgetOverride removed now too
  • it looks like this might remove the slack histogram if I've gone through the diff correctly, it would be nice to have that back

@rowanG077
Copy link
Contributor Author

rowanG077 commented May 26, 2023

  • has your testing covered designs with a critical path from posedge to negedge or vice versa?

Good point will test this out. Do you have a "real world" design that uses this? Of course I can write smaller test cases myself but I want to ensure that it reports the exact same paths even if there are lot's of paths.

  • there's a CI failure as we have -Wall there: error: ‘const char* nextpnr_generic::{anonymous}::edge_name(nextpnr_generic::ClockEdge)’ defined but not used [-Werror=unused-function] 33 | const char *edge_name(ClockEdge edge) { return (edge == FALLING_EDGE) ? "negedge" : "posedge"; }
  • there's another CI failure because himbaechel needs getBudgetOverride removed now too

Right this is still WIP. Code style is also a mess still

it looks like this might remove the slack histogram if I've gone through the diff correctly, it would be nice to have that back

Correct! I need to recover slack histogram and the full critical nets.

  • Implement slack histogram and critical nets
  • Extract timing reporting into timing_report.cc OR refactor report.cc @gatecat. Let me know what you prefer.
  • Further testing

@kittennbfive
Copy link
Contributor

Hi,

will this PR change the critical timing report that nextpnr is providing? (I am asking because i was working on a "beautify"-script for the report, so if the format will change it's pointless for me to continue my work.)

@gatecat
Copy link
Member

gatecat commented May 26, 2023

The plan is for no user visible changes to the report format.

@kittennbfive
Copy link
Contributor

Thank you for this really quick answer!

@rowanG077 rowanG077 force-pushed the remove-budgets branch 2 times, most recently from c56ef56 to e1700da Compare June 9, 2023 09:45
@rowanG077 rowanG077 force-pushed the remove-budgets branch 3 times, most recently from c44e68c to 6fc4bd4 Compare June 20, 2023 08:28
@rowanG077 rowanG077 changed the title WIP: Remove budget based PNR Remove budget based PNR Jun 20, 2023
@rowanG077
Copy link
Contributor Author

rowanG077 commented Jun 20, 2023

@gatecat I have left the timing changes for the next PR. I think this is mergable now.

I have tested a real world ECP5 design with ~70% utilization and it works.

Copy link
Member

@gatecat gatecat left a comment

Choose a reason for hiding this comment

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

LGTM

@gatecat gatecat merged commit 9149996 into YosysHQ:master Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants