-
Notifications
You must be signed in to change notification settings - Fork 894
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
opt_expr: limit effort to reduce runtime on anomalous designs #4782
base: main
Are you sure you want to change the base?
Conversation
passes/opt/opt_expr.cc
Outdated
sort_fails++; | ||
if (sort_fails >= effort) | ||
log("Effort of %d exceeded, no longer attempting toposort on module %s.\n", | ||
effort, log_id(module)); | ||
} | ||
|
||
for (auto cell : cells.sorted) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this empty if you don't run cells.sort()
? And if that's the case, then the true cause of the speed-up isn't not bothering to sort anymore but giving up entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, there's a log_assert(GetSize(sorted) == GetSize(nodes));
right before the return from sort()
so I guess it isn't but I'll verify it later
EDIT: yeah if you don't even run sort()
then it's going to be empty, yikes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by iterating over a list of references to a snapshot of module->cells()
. This increases the runtime to 83 seconds, which I still prefer to 326 seconds. I should say, all runtimes I'm mentioning in this PR are measured with ENABLE_DEBUG=1
for no particular reason other having that around in Makefile.conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could cache the sorting result provided we remove cells we removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really foresee the consequences of doing that, though - for how many cycles would we cache etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the lifetime of one opt_expr
invocation. It should be ok as long as we zero out the position in the sorted array corresponding to any removed cells.
I am not sure if there's a case where opt_expr replaces a cell with two or more cells, that's a slight complication
The first case of the ROM section of
tests/arch/ice40/memories.ys
takes 326 seconds to run with majority of this runtime spent sorting cells inopt_expr
. This PR allows sorting to only fail a limited number of times on a given module before reaching fixpoint without trying to sort the cells. This cuts down the test case runtime to 14 seconds. The limit can be changed with the new-effort
argument.