-
Notifications
You must be signed in to change notification settings - Fork 320
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
[circt-synth] Implemented AIG node balancing algorithm #8262
base: main
Are you sure you want to change the base?
Conversation
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.
Looks really great! Thank you for working on this!! It's really nice to separate the balancing into two separate passes. What do you think replacing the existing LowerVariadic pass with your BlanceVariadic pass? I think it makes sense to me always run if it's always beneficial
I appreciate your feedback and suggestions. Thank you for taking the time to share your review!
The BlanceVariadic pass needs to calculate each node's depth during lowering, it might have runtime issues when the input design has very deep combinational logic. In those kinds of cases, using the existing LowerVariadic will be a good solution to get a usable result in less time. So, I think we should keep the LowerVariadic pass in case anyone has this kind of need. |
if (isa<AndInverterOp>(user)) { | ||
isPO = false; |
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.
This logic is slightly unclear to me. For example I think we should consider %0
is also PO.
%0 = aig.and_inv %port1, not %port2: i1
%1 = aig.and_inv %0, not %port : i1
hw.output %0, %1
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.
You're right; I didn't consider this situation before.
Will change the for loop code like this:
bool isPO = false;
for (auto *user : op->getUsers()) {
if (!isa<AndInverterOp>(user)) {
isPO = true;
break;
}
}
if (isPO)
po.push_back(op);
Is this a bug-free solution?
case 0: | ||
break; | ||
case 1: { | ||
auto signal = sortByLevel.top().second; |
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 think it's necessary to check if the signal is negated.
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.
Thanks for pointing it out. I now add an extra inverter node if the signal is negated. Is it better to push the inverter to its output if the uses are also AIG nodes? (By modifying all uses' inverted array)
This implementation is based on a tree-balancing algorithm described in this paper: "https://ieeexplore.ieee.org/abstract/document/6105357".
Here is an easy-to-understand example (Which has been added as a test case):

The reference algorithm has two stages, "Covering" and "Tree-balancing," divided into
maximum-and-cover
andaig-balance-variadic
passes.A typical usage is:
circt-synth %s --maximum-and-cover --aig-balance-variadic --cse
. Themaximum-and-cover
pass should be always applied before theaig-balance-variadic
pass.