Eliminate double inverts in pyrtl.optimize #462
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #462 +/- ##
=============================================
+ Coverage 91.7% 91.8% +0.2%
=============================================
Files 24 24
Lines 6403 6454 +51
=============================================
+ Hits 5866 5920 +54
+ Misses 537 534 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for fixing this issue! A couple high-level comments before we get into coding details:
- O(N^2) nested
forloops overblock.logicshould generally be avoided in passes because they don't scale well as the number of nets increases - the pass will take too long to run. It seems possible to iterate overblock.logiconce and build a list of all the inverter chains in the block (see how other passes create a_ProducerListfor example). Then I think we can iterate over that list and decide if each inverter chain can be simplified? - This really needs a test :) Some test cases that come to mind:
- Chains of 1, 2, 3, and 4 inverters (even numbers of inverters should be removed, and odd numbers of inverters should be simplified to one inverter)
- A chain of 2 inverters with another user of the first inverter's output (should not be simplified)
gaborszita
force-pushed
the
optimize-double-not
branch
from
3ab98c5 to
6e872b3
Compare
All addressed |
| for net in block.logic: | ||
| if net.op == "~": | ||
| invert_destination_wires[net.dests[0].name] = net | ||
| # If double invert nets are removed randomly, this may leave some double inverts behind. |
There was a problem hiding this comment.
We iterate over the block in topological order (see comment here
Line 595 in 5b282f3
invert_destination_wires in topological order. So this code shouldn't randomly remove inverters in this loop, since dictionaries preserve insertion order, see the bottom of this section https://docs.python.org/3/library/stdtypes.html#typesmapping
| invert_destination_wires = {} | ||
| for net in block.logic: | ||
| if net.op == "~": | ||
| invert_destination_wires[net.dests[0].name] = net |
There was a problem hiding this comment.
I think we can simplify this by mapping from the destination wirevector itself, rather than its name? invert_destination_wires[net.dests[0]] = net
| # If the argument of the net is in invert_destination_wires, then it is a double invert | ||
| # If the net is in net_exclude_set, then it was already removed, so we do not process it | ||
| if net.args[0].name in invert_destination_wires and net not in net_exclude_set: | ||
| previous_net = invert_destination_wires[net.args[0].name] |
There was a problem hiding this comment.
This repeats the dictionary lookup from the previous line, we can avoid one lookup with something like
previous_net = invert_destination_wires.get(net.args[0].name)
if previous_net is not None and ...
I think this also lets you combine this if statement with the next one
| # Dictionary, key is the destination wire of the invert net, value is the invert net | ||
| invert_destination_wires = {} | ||
| for net in block.logic: | ||
| if net.op == "~": |
There was a problem hiding this comment.
Can we remove the used_elsewhere check if we check here for multiple destinations? Something like
if net.op == "~" and len(net.dests) == 1:
| return False | ||
|
|
||
| new_logic = set() | ||
| net_exclude_set = set() # removed nets |
There was a problem hiding this comment.
Seems better to call this removed_nets and remove the comment? :)
| wire_removal_set.add(net.args[0]) | ||
| removed_nets.add(net) | ||
| removed_nets.add(previous_net) | ||
| # remove removed_nets from invert_destination_wires to optimize the for loop |
There was a problem hiding this comment.
I think this is needed to ensure the loop eventually stops (it's not just an optimization?) since line 106 will keep repeating until the net is removed from invert_destination_wires
| if previous_net.args[0].name in invert_destination_wires: | ||
| repeat = True | ||
| else: | ||
| new_logic.add(LogicNet('w', None, args=previous_net.args, dests=net.dests)) |
There was a problem hiding this comment.
This adds a wire net, which can't be removed by _remove_wire_nets since that pass ran already in optimize().
It seems be possible to do this without adding another wire, see how _remove_wire_nets rewrites net.args for other nets. I think we could do something similar here, with a data structure like _ProducerList that tracks even-numbered chains of inverters, and identifies the ultimate source of these chains
|
I will fix these next week, I was busy this weekend. |
Sounds great, no rush! Let me know if you have any questions about my comments, happy to discuss. |
Closes #447
Example: