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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

opt_expr: limit effort to reduce runtime on anomalous designs #4782

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcb3ded
opt_expr: limit effort to reduce runtime on anomalous designs
widlarizer Nov 28, 2024
888e012
opt_expr: don't give up on opt when out of effort
widlarizer Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions passes/opt/opt_expr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ USING_YOSYS_NAMESPACE
PRIVATE_NAMESPACE_BEGIN

bool did_something;
int sort_fails = 0;

void replace_undriven(RTLIL::Module *module, const CellTypes &ct)
{
Expand Down Expand Up @@ -393,7 +394,7 @@ int get_highest_hot_index(RTLIL::SigSpec signal)
return -1;
}

void replace_const_cells(RTLIL::Design *design, RTLIL::Module *module, bool consume_x, bool mux_undef, bool mux_bool, bool do_fine, bool keepdc, bool noclkinv)
void replace_const_cells(RTLIL::Design *design, RTLIL::Module *module, bool consume_x, bool mux_undef, bool mux_bool, bool do_fine, bool keepdc, bool noclkinv, int effort)
{
SigMap assign_map(module);
dict<RTLIL::SigSpec, RTLIL::SigSpec> invert_map;
Expand Down Expand Up @@ -512,10 +513,14 @@ void replace_const_cells(RTLIL::Design *design, RTLIL::Module *module, bool cons
cells.edge(cells.node(outbit_to_cell.at(bit)), r_index);
}

if (!cells.sort()) {
if (sort_fails < effort && !cells.sort()) {
// There might be a combinational loop, or there might be constants on the output of cells. 'check' may find out more.
// ...unless this is a coarse-grained cell loop, but not a bit loop, in which case it won't, and all is good.
log("Couldn't topologically sort cells, optimizing module %s may take a longer time.\n", log_id(module));
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)
Copy link
Member

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

Copy link
Collaborator Author

@widlarizer widlarizer Nov 28, 2024 •
edited
Loading

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

Copy link
Collaborator Author

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

Copy link
Member

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

Copy link
Collaborator Author

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

Copy link
Member

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

Expand Down Expand Up @@ -2253,6 +2258,10 @@ struct OptExprPass : public Pass {
log(" all result bits to be set to x. this behavior changes when 'a+0' is\n");
log(" replaced by 'a'. the -keepdc option disables all such optimizations.\n");
log("\n");
log(" -effort N\n");
log(" allow toposort to fail in N iterations on each module before giving up\n");
log(" on sorting for that module. Default value is 5\n");
log("\n");
}
void execute(std::vector<std::string> args, RTLIL::Design *design) override
{
Expand All @@ -2262,6 +2271,7 @@ struct OptExprPass : public Pass {
bool noclkinv = false;
bool do_fine = false;
bool keepdc = false;
int effort = 5;

log_header(design, "Executing OPT_EXPR pass (perform const folding).\n");
log_push();
Expand Down Expand Up @@ -2299,6 +2309,10 @@ struct OptExprPass : public Pass {
keepdc = true;
continue;
}
if (args[argidx] == "-effort" && argidx + 1 < args.size()) {
effort = atoi(args[++argidx].c_str());
continue;
}
break;
}
extra_args(args, argidx, design);
Expand All @@ -2315,15 +2329,16 @@ struct OptExprPass : public Pass {
design->scratchpad_set_bool("opt.did_something", true);
}

sort_fails = 0;
do {
do {
did_something = false;
replace_const_cells(design, module, false /* consume_x */, mux_undef, mux_bool, do_fine, keepdc, noclkinv);
replace_const_cells(design, module, false /* consume_x */, mux_undef, mux_bool, do_fine, keepdc, noclkinv, effort);
if (did_something)
design->scratchpad_set_bool("opt.did_something", true);
} while (did_something);
if (!keepdc)
replace_const_cells(design, module, true /* consume_x */, mux_undef, mux_bool, do_fine, keepdc, noclkinv);
replace_const_cells(design, module, true /* consume_x */, mux_undef, mux_bool, do_fine, keepdc, noclkinv, effort);
if (did_something)
design->scratchpad_set_bool("opt.did_something", true);
} while (did_something);
Expand Down
Loading