[Buffers] Synchronizing cyle enumeration #688
Conversation
Also it's more modular now, take the logic out of the constructor, and a data flow graph describes a single thing, not an array of things
ziadomalik
force-pushed
the
feat/ziad/synchronizing-cycles
branch
from
ac3944e to
75a54b4
Compare
Jiahui17
left a comment
Jiahui17
left a comment
There was a problem hiding this comment.
Some readability suggestions I missed last time
lib/Transforms/BufferPlacement/LatencyAndOccupancyBalancingSupport.cpp
Outdated
Show resolved
Hide resolved
include/dynamatic/Transforms/BufferPlacement/LatencyAndOccupancyBalancingSupport.h
Outdated
Show resolved
Hide resolved
| /// Find all edges (from nonCyclicAdjList) on any path from cycle to join. | ||
| /// An edge is included if its source is reachable from the cycle and its | ||
| /// destination can reach the join. | ||
| std::vector<size_t> findEdgesToJoin(const SimpleCycle &cycle, | ||
| NodeIdType joinId) const; |
There was a problem hiding this comment.
Explain the returned type (e.g., what is size_t for?)?
There was a problem hiding this comment.
Have you missed this? Here we could write something like:
/// Returns a vector of XXX (here I really don't know what is being returned haha
include/dynamatic/Transforms/BufferPlacement/LatencyAndOccupancyBalancingSupport.h
Show resolved
Hide resolved
| /// Find all edges (from nonCyclicAdjList) on any path from cycle to join. | ||
| /// An edge is included if its source is reachable from the cycle and its | ||
| /// destination can reach the join. | ||
| /// @returns A vector indices into the edges vector. |
There was a problem hiding this comment.
Okay this is why i am a bit confused: is this an id of the edge? If yes we could define another EdgeIdType
There was a problem hiding this comment.
I mean the index is unique to every edge, so in that sense it's an ID. We can also add EdgeIdType
There was a problem hiding this comment.
Also now that I am thinking about it, we could remove id from the Node struct and just use llvm::enumerate everywhere, since it's pretty much an index aswell.
There was a problem hiding this comment.
Done adding a new EdgeIdType!
Let me know if my latter comment is worth the refactor @Jiahui17
See also related PR for Reconvergent paths enumeration.
This feature enumerates synchronizing cycles, as they are defined in Xu, Josipović, FPGA'24, Section 4:
We start out by computing all the strongly-connected-components of the CFC using Kosaraju's algorithm, then we find all the cycles in the CFC using Boost's
tiernan_all_cyclesfunction. Using both of our findings, we can enumerate each pair of cycles, check them against above criteria and reconstruct a path between the pair and the common join if they do match.