tribuf: -nested option: traverse muxes to find nested tribufs#5716
tribuf: -nested option: traverse muxes to find nested tribufs#5716Muxianesty wants to merge 2 commits intoYosysHQ:mainfrom
Conversation
| log(" same net simultaneously. this option implies -merge.\n"); | ||
| log("\n"); | ||
| log(" -nested\n"); | ||
| log(" convert multiplexers using tri-state cells to tri-state cells\n"); |
There was a problem hiding this comment.
I think this help message could be improved, but I'm struggling to come up with a good replacement.
There was a problem hiding this comment.
How about
treat nesting multiplexers with Z-vectors as tri-state cells
?
There was a problem hiding this comment.
It's more complex and specific than that short version. "If a multiplexer is observed outside of a mux tree, convert it to a $tribuf and construct potentially highly complex logic connected to its EN pin to represent the total Z-generation condition" is probably as dense as I can make it. Ultimately, I don't think this will ever have more than one user benefitting from it (you) so it's not a big deal as long as users/devs don't enable it by accident expecting generally useful things to happen
Ravenslofty
left a comment
There was a problem hiding this comment.
This seems okay to me, code-wise, and it seems like a feature that's useful enough that you might want to enable it inside the various synth flows that already call tribuf -logic.
|
After discussing this with the team, this actually seems like a correctness fix. In that scenario, we'd actually prefer this to be the default behaviour, with an option to disable it to restore the old behaviour if really needed. |
|
The idea was to "correctly" gather all tri-state cells to then later convert them into |
| SigSpec sig; | ||
| bool inv; | ||
|
|
||
| TribufSelPart(const SigSpec &sig, bool inv) : sig(sig), inv(inv) |
There was a problem hiding this comment.
You don't need constructors like this if you use curly brace initialization (another C++ bruh moment for the books)
| } | ||
|
|
||
| private: | ||
| struct TribufSelPart { |
There was a problem hiding this comment.
This entire code block seems to be an integral part of the code of the pass. Since it's supporting a niche extra mode, it should be clearly seggregated. TribufSelPart, collect_tribuf_cell... they all seem like code a dev should read when trying to understand the basic function of the pass, which would be the wrong thing to do. You also need to make it clear what you're doing on a logic modeling and algorithmization level. Are you sure you're not doing exactly what we already have in kernel/pattern.h for example?

Judging by how current
tribuf-pass works, only muxes having the "direct" Z-operand are marked as$tribufcells, while in more complex cases Z-operand muxes are used in other muxes, with latter being the "true" tristate cells:In the example above current
tribufpass does not recognize the first mux as a tristate cell.This Pull Request introduces an option
-nested, which traverses muxes in a recursive fashion trying to find the first Z-operand (specifically, the$tribufcell, leading to Z-operand). If found, we construct a logical expression - a conjuntion of all traversed selectors' expressions - to form a condition which "leads" us to the Z-operand, then reverse it. The resulting condition is used as a selector in a newly created$tribufcell. In the example above the expression would be~((~sel) && (~sel2)).To put it simply, we "put forward" the Z-operand to create a "top-level"
$tribufcell to be picked by-merge-like options later.The idea to put this functionality into a separate option rather than being used by default stems from the expansive muxes traversal needing to be done, which in most cases might impact the performance quite a lot.