Optimize multiplexers in Dfg synthesis (#6331)

The previous algorithm was designed to handle the general case where a
full control flow path predicate is required to select which value to
use when synthesizing control flow join point in an always block.

Here we add a better algorithm that tries to use the predicate of
the closest dominating branch if the branch paths dominate the joining
paths. This is almost universally true in synthesizable logic (RTLMeter
has no exceptions), however there are cases where this is not
applicable, for which we fall back on the previous generic algorithm.

Overall this significantly simplifies the synthesized Dfg graphs and
enables further optimization.

src/V3Cfg.cpp

@@ -29,24 +29,9 @@
 VL_DEFINE_DEBUG_FUNCTIONS;
 
 //######################################################################
-// ControlFlowGraph method definitions
+// CfgBlock method definitions
 
-bool ControlFlowGraph::containsLoop() const {
+std::string CfgBlock::name() const {
-    for (const V3GraphVertex& vtx : vertices()) {
-        const BasicBlock& current = static_cast<const BasicBlock&>(vtx);
-        for (const V3GraphEdge& edge : current.outEdges()) {
-            const BasicBlock& successor = *static_cast<const BasicBlock*>(edge.top());
-            // IDs are the reverse post-order numbering, so easy to check for a back-edge
-            if (successor.id() < current.id()) return true;
-        }
-    }
-    return false;
-}
-
-//######################################################################
-// BasicBlock method definitions
-
-std::string BasicBlock::name() const {
     std::stringstream ss;
     ss << "BB " + std::to_string(id()) + ":\n";
     for (AstNode* nodep : m_stmtps) {
@@ -62,25 +47,25 @@ std::string BasicBlock::name() const {
             V3EmitV::debugVerilogForTree(nodep, ss);
         }
     }
-    std::string text = VString::replaceSubstr(ss.str(), "\n", "\\l        ");
+    std::string text = VString::replaceSubstr(
-    if (inEmpty()) text = "**ENTER**\n" + text;
+        VString::replaceSubstr(ss.str(), "\n", "\\l        "), "\"", "\\\"");
-    if (outEmpty()) text = text + "\n**EXIT**";
+    if (isEnter()) text = "**ENTER**\n" + text;
+    if (isExit()) text = text + "\n**EXIT**";
     return text;
 }
-std::string BasicBlock::dotShape() const { return "rect"; }
+std::string CfgBlock::dotShape() const { return "rect"; }
-std::string BasicBlock::dotRank() const {
+std::string CfgBlock::dotRank() const {
-    if (inEmpty()) return "source";
+    if (isEnter()) return "source";
-    if (outEmpty()) return "sink";
+    if (isExit()) return "sink";
     return "";
 }
 
 //######################################################################
-// ControlFlowEdge method definitions
+// CfgEdge method definitions
 
-std::string ControlFlowEdge::dotLabel() const {
+std::string CfgEdge::dotLabel() const {
     std::string label = "E" + std::to_string(id());
-    const BasicBlock& source = *fromp()->as<BasicBlock>();
+    const CfgEdge* const untknp = srcp()->untknEdgep();
-    const ControlFlowEdge* const untknp = source.untknEdgep();
     if (this == untknp) {
         label += " / F";
     } else if (untknp) {
@@ -88,3 +73,249 @@ std::string ControlFlowEdge::dotLabel() const {
     }
     return label;
 }
+
+//######################################################################
+// CfgGraph method definitions
+
+static void cfgOrderVisitBlock(std::vector<CfgBlock*>& postOrderEnumeration, CfgBlock* bbp) {
+    // Mark visited
+    bbp->user(1);
+    // Visit un-visited successors
+    if (CfgBlock* const takenp = bbp->takenp()) {
+        if (!takenp->user()) cfgOrderVisitBlock(postOrderEnumeration, takenp);
+        if (CfgBlock* const untknp = bbp->untknp()) {
+            if (!untknp->user()) cfgOrderVisitBlock(postOrderEnumeration, untknp);
+        }
+    }
+    // Add to post order enumeration
+    postOrderEnumeration.emplace_back(bbp);
+};
+
+void CfgGraph::rpoBlocks() {
+    UASSERT_OBJ(m_nEdits != m_nLastOrdered, m_enterp, "Redundant 'CfgGraph::order' call");
+    m_nLastOrdered = m_nEdits;
+
+    // Reset marks
+    for (V3GraphVertex& v : vertices()) v.user(0);
+
+    // Compute post-order enumeration. Simple recursive algorith will do.
+    std::vector<CfgBlock*> postOrderEnumeration;
+    postOrderEnumeration.reserve(m_nBlocks);
+    cfgOrderVisitBlock(postOrderEnumeration, m_enterp);
+    UASSERT_OBJ(postOrderEnumeration.size() == m_nBlocks, m_enterp, "Inconsistent block count");
+
+    // Assign block IDs equal to the reverse post-order number and sort vertices
+    for (size_t i = 0; i < postOrderEnumeration.size(); ++i) {
+        CfgBlock* const bbp = postOrderEnumeration[m_nBlocks - 1 - i];
+        bbp->m_rpoNumber = i;
+        vertices().unlink(bbp);
+        vertices().linkBack(bbp);
+    }
+
+    // Assign edge IDs
+    size_t edgeCount = 0;
+    for (V3GraphVertex& v : vertices()) {
+        for (V3GraphEdge& e : v.outEdges()) static_cast<CfgEdge&>(e).m_id = edgeCount++;
+    }
+    UASSERT_OBJ(edgeCount == m_nEdges, m_enterp, "Inconsistent edge count");
+}
+
+bool CfgGraph::containsLoop() const {
+    for (const V3GraphVertex& vtx : vertices()) {
+        const CfgBlock& current = static_cast<const CfgBlock&>(vtx);
+        for (const V3GraphEdge& edge : current.outEdges()) {
+            const CfgBlock& successor = *static_cast<const CfgBlock*>(edge.top());
+            // IDs are the reverse post-order numbering, so easy to check for a back-edge
+            if (successor.id() < current.id()) return true;
+        }
+    }
+    return false;
+}
+
+void CfgGraph::minimize() {
+    // Remove empty blocks (except enter and exit)
+    for (V3GraphVertex* const vtxp : vertices().unlinkable()) {
+        CfgBlock* const bbp = static_cast<CfgBlock*>(vtxp);
+        if (bbp->isEnter()) continue;
+        if (bbp->isExit()) continue;
+        if (!bbp->stmtps().empty()) continue;
+        UASSERT(!bbp->isBranch(), "Empty block should have a single successor");
+        CfgBlock* const succp = bbp->takenp();
+        for (V3GraphEdge* const edgep : bbp->inEdges().unlinkable()) edgep->relinkTop(succp);
+        ++m_nEdits;
+        --m_nEdges;
+        --m_nBlocks;
+        VL_DO_DANGLING(bbp->unlinkDelete(this), bbp);
+    }
+
+    // Combine sequential blocks
+    for (V3GraphVertex* const vtxp : vertices().unlinkable()) {
+        CfgBlock* const srcp = static_cast<CfgBlock*>(vtxp);
+        if (srcp->isExit()) continue;
+        if (srcp->isBranch()) continue;
+        CfgBlock* const dstp = srcp->takenp();
+        if (dstp->isJoin()) continue;
+        // Combine them
+        if (srcp->isEnter()) m_enterp = dstp;
+        std::vector<AstNodeStmt*> stmtps{std::move(srcp->m_stmtps)};
+        stmtps.reserve(stmtps.size() + dstp->m_stmtps.size());
+        stmtps.insert(stmtps.end(), dstp->m_stmtps.begin(), dstp->m_stmtps.end());
+        dstp->m_stmtps = std::move(stmtps);
+        for (V3GraphEdge* const edgep : srcp->inEdges().unlinkable()) edgep->relinkTop(dstp);
+        ++m_nEdits;
+        --m_nEdges;
+        --m_nBlocks;
+        VL_DO_DANGLING(srcp->unlinkDelete(this), srcp);
+    }
+
+    if (m_nEdits != m_nLastOrdered) rpoBlocks();
+    if (dumpGraphLevel() >= 9) dumpDotFilePrefixed("cfg-minimize");
+}
+
+void CfgGraph::breakCriticalEdges() {
+    // Gather critical edges
+    std::vector<CfgEdge*> criticalEdges;
+    criticalEdges.reserve(m_nEdges);
+    for (V3GraphVertex& vtx : vertices()) {
+        const CfgBlock& bb = static_cast<const CfgBlock&>(vtx);
+        if (!bb.isBranch()) continue;
+        for (V3GraphEdge& edge : vtx.outEdges()) {
+            const CfgBlock& succ = static_cast<const CfgBlock&>(*edge.top());
+            if (!succ.isJoin()) continue;
+            criticalEdges.emplace_back(static_cast<CfgEdge*>(&edge));
+        }
+    }
+    // Insert blocks
+    for (CfgEdge* const edgep : criticalEdges) {
+        CfgBlock* const newp = addBlock();
+        addTakenEdge(newp, edgep->dstp());
+        edgep->relinkTop(newp);
+    }
+
+    if (m_nEdits != m_nLastOrdered) rpoBlocks();
+    if (dumpGraphLevel() >= 9) dumpDotFilePrefixed("cfg-breakCriticalEdges");
+}
+
+// Given a branching basic block, if the sub-graph below this branch, up until
+// the point where all of its control flow path convertes is series-parallel,
+// then return the (potentially newly created) basic block with exactly 2
+// predecessors where the two control flow paths from this branch have joined.
+// If the relevant sub-graph is not series-parallel (there is a control flow
+// path between the branches, or to a path not dominated by the given branch),
+// then return nullptr. Cached results in the given map
+CfgBlock* CfgGraph::getOrCreateTwoWayJoinFor(CfgBlock* bbp) {
+    UASSERT_OBJ(bbp->isBranch(), bbp, "Not a branch");
+
+    // Mark visited
+    UASSERT_OBJ(!bbp->user(), bbp, "Should not visit twice");
+    bbp->user(1);
+
+    // We need the edge converting to a join block along both path. This is how we find it:
+    const auto chaseEdge = [&](CfgEdge* edgep) -> CfgEdge* {
+        while (true) {
+            CfgBlock* dstp = edgep->dstp();
+            // Stop if found the joining block along this path
+            if (dstp->isJoin()) return edgep;
+            // If the successor is a branch, recursively get it's 2-way join block
+            while (dstp->isBranch()) {
+                dstp = getOrCreateTwoWayJoinFor(dstp);
+                // If the subgarph below dstp is not series-parallel, then no solution
+                if (!dstp) return nullptr;
+            }
+            UASSERT_OBJ(!dstp->isExit(), bbp, "Non-convergent branch - multiple Exit blocks?");
+            edgep = dstp->takenEdgep();
+        }
+    };
+
+    // Walk down both paths
+    CfgEdge* const takenEdgep = chaseEdge(bbp->takenEdgep());
+    if (!takenEdgep) return nullptr;
+    CfgEdge* const untknEdgep = chaseEdge(bbp->untknEdgep());
+    if (!untknEdgep) return nullptr;
+    // If we ended up at different joining blocks, then there is a path from one
+    // of the branches into a path of another branch before 'bbp', no solution
+    if (takenEdgep->dstp() != untknEdgep->dstp()) return nullptr;
+
+    // Pick up the common successor
+    CfgBlock* const succp = takenEdgep->dstp();
+    // If the common successor is a 2-way join, we can use it directly
+    if (succp->isTwoWayJoin()) return succp;
+    // Otherwise insert a new block to join the 2 paths of the original block
+    CfgBlock* const joinp = addBlock();
+    addTakenEdge(joinp, succp);
+    takenEdgep->relinkTop(joinp);
+    untknEdgep->relinkTop(joinp);
+    return joinp;
+}
+
+bool CfgGraph::insertTwoWayJoins() {
+    // Reset marks
+    for (V3GraphVertex& v : vertices()) v.user(0);
+
+    bool isSeriesParallel = true;
+
+    // We will be adding vertices at the end. That's OK, they don't need to be visited again
+    for (V3GraphVertex& v : vertices()) {
+        CfgBlock& bb = static_cast<CfgBlock&>(v);
+        // Skip if already visited
+        if (bb.user()) continue;
+        // Skip if not a branch
+        if (!bb.isBranch()) continue;
+        // Fix it up, record if failed
+        if (!getOrCreateTwoWayJoinFor(&bb)) isSeriesParallel = false;
+    }
+
+    if (m_nEdits != m_nLastOrdered) rpoBlocks();
+    if (dumpGraphLevel() >= 9) dumpDotFilePrefixed("cfg-insertTwoWayJoins");
+
+    return isSeriesParallel;
+}
+
+//######################################################################
+// CfgDominatorTree
+
+const CfgBlock* CfgDominatorTree::intersect(const CfgBlock* ap, const CfgBlock* bp) {
+    while (ap != bp) {
+        while (*ap > *bp) ap = m_bb2Idom[*ap];
+        while (*bp > *ap) bp = m_bb2Idom[*bp];
+    }
+    return ap;
+}
+
+CfgDominatorTree::CfgDominatorTree(const CfgGraph& cfg)
+    : m_bb2Idom{cfg.makeBlockMap<const CfgBlock*>()} {
+
+    // Build the immediate dominator map, using algorithm from:
+    // "A Simple, Fast Dominance Algorithm", Keith D. Cooper et al., 2006
+    // Immediate dominator of the enter block
+
+    // Point enteer block to itself, while computing below
+    m_bb2Idom[cfg.enter()] = &cfg.enter();
+    // Iterate until settled
+    for (bool changed = true; changed;) {
+        changed = false;
+        // For each vertex except enter block
+        for (const V3GraphVertex& vtx : cfg.vertices()) {
+            const CfgBlock& curr = static_cast<const CfgBlock&>(vtx);
+            if (curr.isEnter()) continue;  // Skip entry block
+
+            // For each predecessor of current block
+            const CfgBlock* idom = nullptr;
+            for (const V3GraphEdge& edge : curr.inEdges()) {
+                const CfgBlock& pred = static_cast<const CfgBlock&>(*edge.fromp());
+                // Skip if perdecessor not yet processed
+                if (!m_bb2Idom[pred]) continue;
+                // Pick first, then use intersect
+                idom = !idom ? &pred : intersect(&pred, idom);
+            }
+
+            // If chenged, record it, else move on
+            if (idom == m_bb2Idom[curr]) continue;
+            m_bb2Idom[curr] = idom;
+            changed = true;
+        }
+    }
+
+    // The enter block is the root of the tree and does not itself have an immediate dominator
+    m_bb2Idom[cfg.enter()] = nullptr;
+}

src/V3Cfg.h

@@ -68,69 +68,103 @@
 #include <vector>
 
 //######################################################################
-// DenseMap - This can be made generic if needed
+// Control Flow Graph (CFG) and related data structures
 
-// Map from unique non-negative integers (IDs) to generic values. As opposed
+class CfgGraph;
-// to a std::map/std::unordered_map, in DenseMap, all entries are value
+class CfgBlock;
-// initialized at construction, and the map is always dense. This can be
+class CfgEdge;
-// improved if necessary but is sufficient for our current purposes.
-template <typename T_Key, size_t (T_Key::*Index)() const, typename T_Value>
-class DenseMap final {
-    std::vector<T_Value> m_map;  // The map, stored as a vector
 
-public:
+template <typename T_Key, typename T_Value>
-    // CONSTRUCTOR
+class CfgMap;
-    explicit DenseMap(size_t size)
+template <typename T_Value>
-        : m_map{size} {}
+class CfgBlockMap;
-
+template <typename T_Value>
-    T_Value& operator[](const T_Key& key) { return m_map.at((key.*Index)()); }
+class CfgEdgeMap;
-    const T_Value& operator[](const T_Key& key) const { return m_map.at((key.*Index)()); }
-};
 
 //######################################################################
-// ControlFlowGraph data structure
+// CfgBlock - A basic block (verticies of the control flow graph)
 
-class ControlFlowGraph;
+class CfgBlock final : public V3GraphVertex {
-class BasicBlock;
+    friend class CfgGraph;
-class ControlFlowEdge;
+    template <typename T_Key, typename T_Value>
-
+    friend class CfgMap;
-// A basic block (verticies of the control flow graph)
-class BasicBlock final : public V3GraphVertex {
-    VL_RTTI_IMPL(BasicBlock, V3GraphVertex)
     friend class CfgBuilder;
 
-    // STATE - Immutable after construction, set by CfgBuilder
+    // STATE
-    // V3GraphEdge::user() is set to the unique id by CfgBuilder
+    CfgGraph* const m_cfgp;  // The control flow graph this CfgBlock is under
-    std::vector<AstNodeStmt*> m_stmtps;  // statements in this BasicBlock
+    size_t m_rpoNumber;  // Reverse post-order number and unique ID of this CfgBlock
 
-    // CONSTRUCTOR - via CfgBuilder only
+    // V3GraphEdge::user() is set to the unique id by CfgBuilder
-    inline explicit BasicBlock(ControlFlowGraph* graphp);
+    std::vector<AstNodeStmt*> m_stmtps;  // statements in this CfgBlock
-    ~BasicBlock() override = default;
+
+    // PRIVATE METHODS
+    // ID (reverse post-order numpber) of this block
+    inline size_t id();
+    inline size_t id() const;
+
+    // CONSTRUCTOR/DESTRUCTOR - via CfgGraph only
+    inline explicit CfgBlock(CfgGraph* cfgp);
+    ~CfgBlock() override = default;
 
 public:
-    // METHODS - all const
+    // PUBLIC METHODS
 
-    // Statements in this BasicBlock
+    // Is this the entry block of the CFG?
+    bool isEnter() const { return inEmpty(); }
+    // Is this the exit block of the CFG?
+    bool isExit() const { return outEmpty(); }
+    // Is this a branching block (multiple successors)?
+    bool isBranch() const { return outEdges().hasMultipleElements(); }
+    // Is this a control flow convergence block (multiple predecessors)?
+    bool isJoin() const { return inEdges().hasMultipleElements(); }
+    // Is this a join of exactly 2 paths?
+    bool isTwoWayJoin() const { return inEdges().hasTwoElements(); }
+
+    // The edge going to the taken (or uncinditional) successor, or nullptr if exit block
+    inline CfgEdge* takenEdgep();
+    inline const CfgEdge* takenEdgep() const;
+    // The edge going to the untaken successor, or nullptr if not a branch, or exit block
+    inline CfgEdge* untknEdgep();
+    inline const CfgEdge* untknEdgep() const;
+    // The taken successor block, or nullptr if exit block
+    inline CfgBlock* takenp();
+    inline const CfgBlock* takenp() const;
+    // The untakens successor block, or nullptr if not a branch, or exit block
+    inline CfgBlock* untknp();
+    inline const CfgBlock* untknp() const;
+
+    // The first predecessor edge, or nullptr if enter block
+    inline CfgEdge* firstPredecessorEdgep();
+    inline const CfgEdge* firstPredecessorEdgep() const;
+    // The last predecessor edge, or nullptr if enter block
+    inline CfgEdge* lastPredecessorEdgep();
+    inline const CfgEdge* lastPredecessorEdgep() const;
+    // The first predecessor block, or nullptr if enter block
+    inline CfgBlock* firstPredecessorp();
+    inline const CfgBlock* firstPredecessorp() const;
+    // The last predecessor block, or nullptr if enter block
+    inline CfgBlock* lastPredecessorp();
+    inline const CfgBlock* lastPredecessorp() const;
+
+    // Statements in this CfgBlock
     const std::vector<AstNodeStmt*>& stmtps() const { return m_stmtps; }
-    // Unique ID of this BasicBlock - defines topological ordering
-    size_t id() const { return V3GraphVertex::user(); }
 
-    // The edge corresponding to the terminator branch being taken (including unonditoinal goto)
+    // Ordering is done on reverese post-order numbering. For loop-free graph
-    inline const ControlFlowEdge* takenEdgep() const;
+    // this ensures that a block that compares less than another is not a
-    // The edge corresponding to the terminator branch being not taken (or nullptr if goto)
+    // successor of the other block (it is an ancestor, or sibling).
-    inline const ControlFlowEdge* untknEdgep() const;
+    bool operator<(const CfgBlock& that) const { return id() < that.id(); }
-    // Same as takenpEdgep/untknEdgep but returns the successor basic blocks
+    bool operator>(const CfgBlock& that) const { return id() > that.id(); }
-    inline const BasicBlock* takenSuccessorp() const;
+    bool operator==(const CfgBlock& that) const { return this == &that; }
-    inline const BasicBlock* untknSuccessorp() const;
 
-    void forEachSuccessor(std::function<void(const BasicBlock&)> f) const {
+    // Iterators
-        for (const V3GraphEdge& edge : outEdges()) f(*edge.top()->as<BasicBlock>());
+    void forEachSuccessor(std::function<void(const CfgBlock&)> f) const {
-    }
+        for (const V3GraphEdge& edge : outEdges()) f(*static_cast<CfgBlock*>(edge.top()));
-
+    }
-    void forEachPredecessor(std::function<void(const BasicBlock&)> f) const {
+    void forEachPredecessor(std::function<void(const CfgBlock&)> f) const {
-        for (const V3GraphEdge& edge : inEdges()) f(*edge.fromp()->as<BasicBlock>());
+        for (const V3GraphEdge& edge : inEdges()) f(*static_cast<CfgBlock*>(edge.fromp()));
     }
 
+    // Source location for debugging
     FileLine* fileline() const override {
         return !m_stmtps.empty() ? m_stmtps.front()->fileline() : nullptr;
     }
@@ -141,130 +175,397 @@ public:
     std::string dotRank() const override;
 };
 
-// A control flow graph edge
+//######################################################################
-class ControlFlowEdge final : public V3GraphEdge {
+// CfgEdge - An edges of the control flow graph
-    VL_RTTI_IMPL(ControlFlowEdge, V3GraphEdge)
+
-    friend class CfgBuilder;
+class CfgEdge final : public V3GraphEdge {
+    friend class CfgGraph;
+    template <typename T_Key, typename T_Value>
+    friend class CfgMap;
 
     // STATE - Immutable after construction, set by CfgBuilder
-    // V3GraphEdge::user() is set to the unique id by CfgBuilder
+    CfgGraph* const m_cfgp;  // The control flow graph this CfgEdge is under
+    size_t m_id;  // Unique ID of this vertex
 
-    // CONSTRUCTOR - via CfgBuilder only
+    // PRIVATE METHODS
-    inline ControlFlowEdge(ControlFlowGraph* graphp, BasicBlock* srcp, BasicBlock* dstp);
+    // Unique ID of this CfgEdge - no particular meaning
-    ~ControlFlowEdge() override = default;
+    inline size_t id();
+    inline size_t id() const;
+
+    // CONSTRUCTOR/DESTRUCTOR - via CfgGraph only
+    inline CfgEdge(CfgGraph* graphp, CfgBlock* srcp, CfgBlock* dstp);
+    ~CfgEdge() override = default;
 
 public:
     // METHODS - all const
 
-    // Unique ID of this ControlFlowEdge - no particular meaning
+    // Source/destination CfgBlock
-    size_t id() const { return user(); }
+    const CfgBlock* srcp() const { return static_cast<const CfgBlock*>(fromp()); }
-
+    const CfgBlock* dstp() const { return static_cast<const CfgBlock*>(top()); }
-    // Source/destination BasicBlock
+    CfgBlock* srcp() { return static_cast<CfgBlock*>(fromp()); }
-    const BasicBlock& src() const { return *static_cast<BasicBlock*>(fromp()); }
+    CfgBlock* dstp() { return static_cast<CfgBlock*>(top()); }
-    const BasicBlock& dst() const { return *static_cast<BasicBlock*>(top()); }
 
     // For Graphviz dumps only
     std::string dotLabel() const override;
 };
 
-template <typename T_Value>
+//######################################################################
-using BasicBlockMap = DenseMap<BasicBlock, &BasicBlock::id, T_Value>;
+// CfgGraph - The control flow graph
 
-template <typename T_Value>
+class CfgGraph final : public V3Graph {
-using ControlFlowEdgeMap = DenseMap<ControlFlowEdge, &ControlFlowEdge::id, T_Value>;
+    friend class CfgBlock;
-
+    friend class CfgEdge;
-// The control flow graph
+    template <typename T_Key, typename T_Value>
-class ControlFlowGraph final : public V3Graph {
+    friend class CfgMap;
     friend class CfgBuilder;
 
-    // STATE - Immutable after construction, set by CfgBuilder
+    // STATE
-    BasicBlock* m_enterp = nullptr;  // The singular entry vertex
+    size_t m_nEdits = 0;  // Edit count of this graph
-    BasicBlock* m_exitp = nullptr;  // The singular exit vertex
+    size_t m_nLastOrdered = 0;  // Last edit count blocks were ordered
-    size_t m_nBasicBlocks = 0;  // Number of BasicBlocks in this ControlFlowGraph
+    CfgBlock* m_enterp = nullptr;  // The singular entry vertex
-    size_t m_nEdges = 0;  // Number of ControlFlowEdges in this ControlFlowGraph
+    CfgBlock* m_exitp = nullptr;  // The singular exit vertex
+    size_t m_nBlocks = 0;  // Number of CfgBlocks in this CfgGraph
+    size_t m_nEdges = 0;  // Number of CfgEdges in this CfgGraph
 
-    // CONSTRUCTOR - via CfgBuilder only
+    // PRIVATE METHODS
-    ControlFlowGraph() = default;
 
-public:
+    // Compute reverse post-order enumeration of blocks, and sort them
-    ~ControlFlowGraph() override = default;
+    // accordingly. Assign blocks, and edge IDs. Invalidates all previous IDs.
+    void rpoBlocks();
 
-    // METHODS
+    // Add a new CfgBlock to this graph
-    void foreach(std::function<void(const BasicBlock&)> f) const {
+    CfgBlock* addBlock() {
-        for (const V3GraphVertex& vtx : vertices()) f(*vtx.as<BasicBlock>());
+        ++m_nEdits;
+        ++m_nBlocks;
+        return new CfgBlock{this};
     }
 
+    // Add a new taken (or unconditional) CfgEdge to this CFG
+    void addTakenEdge(CfgBlock* srcp, CfgBlock* dstp) {
+        UASSERT_OBJ(srcp->m_cfgp == this, srcp, "'srcp' is not in this graph");
+        UASSERT_OBJ(dstp->m_cfgp == this, dstp, "'dstp' is not in this graph");
+        UASSERT_OBJ(!srcp->takenEdgep(), srcp, "Taken edge should be added first");
+        //
+        UASSERT_OBJ(dstp != m_enterp, dstp, "Enter block cannot have a predecessor");
+        UASSERT_OBJ(srcp != m_exitp, srcp, "Exit block cannot have a successor");
+        ++m_nEdits;
+        ++m_nEdges;
+        new CfgEdge{this, srcp, dstp};
+    }
+
+    // Add a new untaken CfgEdge to this CFG
+    void addUntknEdge(CfgBlock* srcp, CfgBlock* dstp) {
+        UASSERT_OBJ(srcp->m_cfgp == this, srcp, "'srcp' is not in this graph");
+        UASSERT_OBJ(dstp->m_cfgp == this, dstp, "'dstp' is not in this graph");
+        UASSERT_OBJ(srcp->takenEdgep(), srcp, "Untaken edge shold be added second");
+        UASSERT_OBJ(srcp->takenp() != dstp, srcp, "Untaken branch targets the same block");
+        //
+        UASSERT_OBJ(dstp != m_enterp, dstp, "Enter block cannot have a predecessor");
+        UASSERT_OBJ(srcp != m_exitp, srcp, "Exit block cannot have a successor");
+        ++m_nEdits;
+        ++m_nEdges;
+        new CfgEdge{this, srcp, dstp};
+    }
+
+    size_t idOf(const CfgBlock* bbp) const {
+        UASSERT_OBJ(m_nEdits == m_nLastOrdered, m_enterp, "Cfg was edited but not re-ordered");
+        return bbp->m_rpoNumber;
+    }
+
+    size_t idOf(const CfgEdge* edgep) const {
+        UASSERT_OBJ(m_nEdits == m_nLastOrdered, m_enterp, "Cfg was edited but not re-ordered");
+        return edgep->m_id;
+    }
+
+    // Implementation for insertTwoWayJoins
+    CfgBlock* getOrCreateTwoWayJoinFor(CfgBlock* bbp);
+
+    // CONSTRUCTOR - use CfgGraph::build, which might fail, so this can't be public
+    CfgGraph() = default;
+
+public:
+    ~CfgGraph() override = default;
+
+    // STATIC FUNCTIONS
+
+    // Build CFG for the given list of statements
+    static std::unique_ptr<CfgGraph> build(AstNode* stmtsp);
+
+    // PUBLIC METHODS
+
     // Accessors
-    const BasicBlock& enter() const { return *m_enterp; }
+    const CfgBlock& enter() const { return *m_enterp; }
-    const BasicBlock& exit() const { return *m_exitp; }
+    const CfgBlock& exit() const { return *m_exitp; }
 
     // Number of basic blocks in this graph
-    size_t nBasicBlocks() const { return m_nBasicBlocks; }
+    size_t nBlocks() const { return m_nBlocks; }
     // Number of control flow edges in this graph
     size_t nEdges() const { return m_nEdges; }
 
-    // Create a BasicBlock map for this graph
+    // Create a CfgBlock map for this graph
     template <typename T_Value>
-    BasicBlockMap<T_Value> makeBasicBlockMap() const {
+    inline CfgBlockMap<T_Value> makeBlockMap() const;
-        return BasicBlockMap<T_Value>{nBasicBlocks()};
+    // Create a CfgEdgeMap map for this graph
-    }
-    // Create a ControlFlowEdgeMap map for this graph
     template <typename T_Value>
-    ControlFlowEdgeMap<T_Value> makeEdgeMap() const {
+    inline CfgEdgeMap<T_Value> makeEdgeMap() const;
-        return ControlFlowEdgeMap<T_Value>{nEdges()};
-    }
 
     // Returns true iff the graph contains a loop (back-edge)
     bool containsLoop() const;
+
+    //------------------------------------------------------------------
+    // The following methods mutate this CFG and invalidate CfgBlock and
+    // CfgEdge IDs and associated CfgBlockMap, CfgEdgeMap and other
+    // query instances.
+
+    // Remove empty blocks, combine sequential blocks. Keeps the enter/exit block, even if empty.
+    void minimize();
+
+    // Insert empty blocks to fix critical edges (edges that have a source with
+    // multiple successors, and a destination with multiple predecessors)
+    void breakCriticalEdges();
+
+    bool insertTwoWayJoins();
 };
 
 //######################################################################
-// Inline method definitions
+// CfgMap - Map from CfgBlock or CfgEdge to generic values
 
-BasicBlock::BasicBlock(ControlFlowGraph* cfgp)
+template <typename T_Key, typename T_Value>
-    : V3GraphVertex{cfgp} {}
+class CfgMap VL_NOT_FINAL {
+    // As opposed to a std::map/std::unordered_map, all entries are value
+    // initialized at construction, and the map is always dense. This can
+    // be improved if necessary but is sufficient for our current purposes.
 
-const ControlFlowEdge* BasicBlock::takenEdgep() const {
+    const CfgGraph* m_cfgp;  // The control flow graph this map is for
-    // It's always the first edge
+    size_t m_created;  // Edit count of CFG this map was created at
-    const V3GraphEdge* const frontp = outEdges().frontp();
+    std::vector<T_Value> m_map;  // The map, stored as a vector
-    return static_cast<const ControlFlowEdge*>(frontp);
-}
 
-const ControlFlowEdge* BasicBlock::untknEdgep() const {
+protected:
-    // It's always the second (last) edge
+    // CONSTRUCTOR
-    const V3GraphEdge* const frontp = outEdges().frontp();
+    explicit CfgMap(const CfgGraph* cfgp, size_t size)
-    const V3GraphEdge* const backp = outEdges().backp();
+        : m_cfgp{cfgp}
-    return backp != frontp ? static_cast<const ControlFlowEdge*>(backp) : nullptr;
+        , m_created{cfgp->m_nEdits}
-}
+        , m_map{size} {}
 
-const BasicBlock* BasicBlock::takenSuccessorp() const {
+public:
-    const ControlFlowEdge* const edgep = takenEdgep();
+    // Can create an empty map
-    return edgep ? static_cast<const BasicBlock*>(edgep->top()) : nullptr;
+    CfgMap()
-}
+        : m_cfgp{nullptr}
+        , m_created{0} {}
 
-const BasicBlock* BasicBlock::untknSuccessorp() const {
+    // Copyable, movable
-    const ControlFlowEdge* const edgep = untknEdgep();
+    CfgMap(const CfgMap<T_Key, T_Value>&) = default;
-    return edgep ? static_cast<const BasicBlock*>(edgep->top()) : nullptr;
+    CfgMap(CfgMap<T_Key, T_Value>&&) = default;
-}
+    CfgMap<T_Key, T_Value>& operator=(const CfgMap<T_Key, T_Value>&) = default;
+    CfgMap<T_Key, T_Value>& operator=(CfgMap<T_Key, T_Value>&&) = default;
 
-ControlFlowEdge::ControlFlowEdge(ControlFlowGraph* graphp, BasicBlock* srcp, BasicBlock* dstp)
+    T_Value& operator[](const T_Key& key) {
-    : V3GraphEdge{graphp, srcp, dstp, 1, false} {}
+        UASSERT_OBJ(m_created == m_cfgp->m_nEdits, m_cfgp->m_enterp, "Map is stale");
+        UASSERT_OBJ(m_cfgp == key.m_cfgp, m_cfgp->m_enterp, "Key not in this CFG");
+        return m_map.at(key.id());
+    }
+    const T_Value& operator[](const T_Key& key) const {
+        UASSERT_OBJ(m_created == m_cfgp->m_nEdits, m_cfgp->m_enterp, "Map is stale");
+        UASSERT_OBJ(m_cfgp == key.m_cfgp, m_cfgp->m_enterp, "Key not in this CFG");
+        return m_map.at(key.id());
+    }
+    T_Value& operator[](const T_Key* keyp) {
+        UASSERT_OBJ(m_created == m_cfgp->m_nEdits, m_cfgp->m_enterp, "Map is stale");
+        UASSERT_OBJ(m_cfgp == keyp->m_cfgp, m_cfgp->m_enterp, "Key not in this CFG");
+        return m_map.at(keyp->id());
+    }
+    const T_Value& operator[](const T_Key* keyp) const {
+        UASSERT_OBJ(m_created == m_cfgp->m_nEdits, m_cfgp->m_enterp, "Map is stale");
+        UASSERT_OBJ(m_cfgp == keyp->m_cfgp, m_cfgp->m_enterp, "Key not in this CFG");
+        return m_map.at(keyp->id());
+    }
+};
+
+template <typename T_Value>
+class CfgBlockMap final : public CfgMap<CfgBlock, T_Value> {
+    friend class CfgGraph;
+    // CONSTRUCTOR - Create one via CfgGraph::makeBlockMap
+    explicit CfgBlockMap(const CfgGraph* cfgp)
+        : CfgMap<CfgBlock, T_Value>{cfgp, cfgp->nBlocks()} {}
+
+public:
+    // Can create an empty map
+    CfgBlockMap() = default;
+    // Copyable, movable
+    CfgBlockMap(const CfgBlockMap&) = default;
+    CfgBlockMap(CfgBlockMap&&) = default;
+    CfgBlockMap& operator=(const CfgBlockMap&) = default;
+    CfgBlockMap& operator=(CfgBlockMap&&) = default;
+};
+
+template <typename T_Value>
+class CfgEdgeMap final : public CfgMap<CfgEdge, T_Value> {
+    friend class CfgGraph;
+    // CONSTRUCTOR - Create one via CfgGraph::makeEdgeMap
+    explicit CfgEdgeMap(const CfgGraph* cfgp)
+        : CfgMap<CfgEdge, T_Value>{cfgp, cfgp->nEdges()} {}
+
+public:
+    // Can create an empty map
+    CfgEdgeMap() = default;
+    // Copyable, movable
+    CfgEdgeMap(const CfgEdgeMap&) = default;
+    CfgEdgeMap(CfgEdgeMap&&) = default;
+    CfgEdgeMap& operator=(const CfgEdgeMap&) = default;
+    CfgEdgeMap& operator=(CfgEdgeMap&&) = default;
+};
 
 //######################################################################
-// ControlFlowGraph functions
+// Innline method definitions
+
+// --- CfgBlock ---
+
+CfgBlock::CfgBlock(CfgGraph* cfgp)
+    : V3GraphVertex{cfgp}
+    , m_cfgp{cfgp} {}
+
+size_t CfgBlock::id() { return m_cfgp->idOf(this); }
+size_t CfgBlock::id() const { return m_cfgp->idOf(this); }
+
+// Successor edges
+CfgEdge* CfgBlock::takenEdgep() {  // It's always the first edge
+    return isExit() ? nullptr : static_cast<CfgEdge*>(outEdges().frontp());
+}
+const CfgEdge* CfgBlock::takenEdgep() const {  // It's always the first edge
+    return isExit() ? nullptr : static_cast<const CfgEdge*>(outEdges().frontp());
+}
+CfgEdge* CfgBlock::untknEdgep() {  // It's always the second (last) edge
+    return isBranch() ? static_cast<CfgEdge*>(outEdges().backp()) : nullptr;
+}
+const CfgEdge* CfgBlock::untknEdgep() const {  // It's always the second (last) edge
+    return isBranch() ? static_cast<const CfgEdge*>(outEdges().backp()) : nullptr;
+}
+CfgBlock* CfgBlock::takenp() {
+    return isExit() ? nullptr : static_cast<CfgBlock*>(outEdges().frontp()->top());
+}
+const CfgBlock* CfgBlock::takenp() const {
+    return isExit() ? nullptr : static_cast<CfgBlock*>(outEdges().frontp()->top());
+}
+CfgBlock* CfgBlock::untknp() {
+    return isBranch() ? static_cast<CfgBlock*>(outEdges().backp()->top()) : nullptr;
+}
+const CfgBlock* CfgBlock::untknp() const {
+    return isBranch() ? static_cast<const CfgBlock*>(outEdges().backp()->top()) : nullptr;
+}
+
+// Predecessor edges
+CfgEdge* CfgBlock::firstPredecessorEdgep() {  //
+    return static_cast<CfgEdge*>(inEdges().frontp());
+}
+const CfgEdge* CfgBlock::firstPredecessorEdgep() const {  //
+    return static_cast<const CfgEdge*>(inEdges().frontp());
+}
+CfgEdge* CfgBlock::lastPredecessorEdgep() {  //
+    return static_cast<CfgEdge*>(inEdges().backp());
+}
+const CfgEdge* CfgBlock::lastPredecessorEdgep() const {  //
+    return static_cast<const CfgEdge*>(inEdges().backp());
+}
+CfgBlock* CfgBlock::firstPredecessorp() {  //
+    return isEnter() ? nullptr : firstPredecessorEdgep()->srcp();
+}
+const CfgBlock* CfgBlock::firstPredecessorp() const {  //
+    return isEnter() ? nullptr : firstPredecessorEdgep()->srcp();
+}
+CfgBlock* CfgBlock::lastPredecessorp() {  //
+    return isEnter() ? nullptr : lastPredecessorEdgep()->srcp();
+}
+const CfgBlock* CfgBlock::lastPredecessorp() const {  //
+    return isEnter() ? nullptr : lastPredecessorEdgep()->srcp();
+}
+
+// --- CfgEdge ---
+
+CfgEdge::CfgEdge(CfgGraph* cfgp, CfgBlock* srcp, CfgBlock* dstp)
+    : V3GraphEdge{cfgp, srcp, dstp, 1, false}
+    , m_cfgp{cfgp} {}
+
+size_t CfgEdge::id() { return m_cfgp->idOf(this); }
+size_t CfgEdge::id() const { return m_cfgp->idOf(this); }
+
+// --- CfgGraph ---
+
+template <typename T_Value>
+CfgBlockMap<T_Value> CfgGraph::makeBlockMap() const {
+    return CfgBlockMap<T_Value>{this};
+}
+template <typename T_Value>
+CfgEdgeMap<T_Value> CfgGraph::makeEdgeMap() const {
+    return CfgEdgeMap<T_Value>{this};
+}
+
+//######################################################################
+// CfgDominatorTree
+
+class CfgDominatorTree final {
+    // STATE
+    CfgBlockMap<const CfgBlock*> m_bb2Idom;  // Map from CfgBlock to its immediate dominator
+
+    // PRIVATE METHODS
+
+    // Part of algorithm to compute m_bb2Idom, see consructor
+    const CfgBlock* intersect(const CfgBlock* ap, const CfgBlock* bp);
+
+public:
+    // CONSTRUCTOR
+    explicit CfgDominatorTree(const CfgGraph& cfg);
+    // Can create an empty map
+    CfgDominatorTree() = default;
+    // Copyable, movable
+    CfgDominatorTree(const CfgDominatorTree&) = default;
+    CfgDominatorTree(CfgDominatorTree&&) = default;
+    CfgDominatorTree& operator=(const CfgDominatorTree&) = default;
+    CfgDominatorTree& operator=(CfgDominatorTree&&) = default;
+
+    // PUBLIC METHODS
+
+    // Return unique CfgBlock that dominates both of the given blocks, but does
+    // not strictly dominate any other block that dominates both blocks. It
+    // will return 'ap' or 'bp' if one dominates the other (or are the same)
+    const CfgBlock* closestCommonDominator(const CfgBlock* ap, const CfgBlock* bp) const {
+        while (ap != bp) {
+            if (*ap < *bp) {
+                bp = m_bb2Idom[bp];
+            } else {
+                ap = m_bb2Idom[ap];
+            }
+        }
+        return ap;
+    }
+
+    // Returns true if 'ap' dominates 'bp'
+    bool dominates(const CfgBlock* const ap, const CfgBlock* bp) {
+        // Walk up the dominator tree from 'bp' until reaching 'ap' or the root
+        while (true) {
+            // True if 'ap' is above (or same as) 'bp'
+            if (ap == bp) return true;
+            // Step up the dominator tree
+            bp = m_bb2Idom[bp];
+            // False if reached the root
+            if (!bp) return false;
+        }
+    }
+};
+
+//######################################################################
+// V3Cfg
 
 namespace V3Cfg {
-// Build control flow graph for given node
-std::unique_ptr<const ControlFlowGraph> build(const AstNodeProcedure*);
 
 // Compute AstVars live on entry to given CFG. That is, variables that might
 // be read before wholly assigned in the CFG. Returns nullptr if the analysis
 // failed due to unhandled statements or data types involved in the CFG.
 // On success, returns a vector of AstVar or AstVarScope nodes live on entry.
-std::unique_ptr<std::vector<AstVar*>> liveVars(const ControlFlowGraph&);
+std::unique_ptr<std::vector<AstVar*>> liveVars(const CfgGraph&);
 
 // Same as liveVars, but return AstVarScopes insted
-std::unique_ptr<std::vector<AstVarScope*>> liveVarScopes(const ControlFlowGraph&);
+std::unique_ptr<std::vector<AstVarScope*>> liveVarScopes(const CfgGraph&);
+
 }  //namespace V3Cfg
 
 #endif  // VERILATOR_V3CFG_H_

src/V3CfgBuilder.cpp

@@ -31,35 +31,18 @@
 
 VL_DEFINE_DEBUG_FUNCTIONS;
 
-class CfgBuilder final : VNVisitorConst {
+class CfgBuilder final : public VNVisitorConst {
     // STATE
     // The graph being built, or nullptr if failed to build one
-    std::unique_ptr<ControlFlowGraph> m_cfgp{new ControlFlowGraph};
+    std::unique_ptr<CfgGraph> m_cfgp{new CfgGraph};
     // Current basic block to add statements to
-    BasicBlock* m_currBBp = nullptr;
+    CfgBlock* m_currBBp = nullptr;
     // Continuation block for given JumpBlock
-    std::unordered_map<AstJumpBlock*, BasicBlock*> m_jumpBlockContp;
+    std::unordered_map<AstJumpBlock*, CfgBlock*> m_jumpBlockContp;
 
     // METHODS
 
-    // Create new Basicblock block in the CFG
+    // Add the given statement to the current CfgBlock
-    BasicBlock& addBasicBlock() { return *new BasicBlock{m_cfgp.get()}; }
-
-    // Create new taken (or unconditional) ControlFlowEdge in the CFG
-    void addTakenEdge(BasicBlock& src, BasicBlock& dst) {
-        UASSERT_OBJ(src.outEmpty(), &src, "Taken edge should be added first");
-        new ControlFlowEdge{m_cfgp.get(), &src, &dst};
-    }
-
-    // Create new untaken ControlFlowEdge in the CFG
-    void addUntknEdge(BasicBlock& src, BasicBlock& dst) {
-        UASSERT_OBJ(src.outSize1(), &src, "Untaken edge shold be added second");
-        UASSERT_OBJ(src.outEdges().frontp()->top() != &dst, &src,
-                    "Untaken branch targets the same block as the taken branch");
-        new ControlFlowEdge{m_cfgp.get(), &src, &dst};
-    }
-
-    // Add the given statement to the current BasicBlock
     void addStmt(AstNodeStmt* nodep) { m_currBBp->m_stmtps.emplace_back(nodep); }
 
     // Used to handle statements not representable in the CFG
@@ -115,71 +98,71 @@ class CfgBuilder final : VNVisitorConst {
         addStmt(nodep);
 
         // Create then/else/continuation blocks
-        BasicBlock& thenBB = addBasicBlock();
+        CfgBlock* const thenBBp = m_cfgp->addBlock();
-        BasicBlock& elseBB = addBasicBlock();
+        CfgBlock* const elseBBp = m_cfgp->addBlock();
-        BasicBlock& contBB = addBasicBlock();
+        CfgBlock* const contBBp = m_cfgp->addBlock();
-        addTakenEdge(*m_currBBp, thenBB);
+        m_cfgp->addTakenEdge(m_currBBp, thenBBp);
-        addUntknEdge(*m_currBBp, elseBB);
+        m_cfgp->addUntknEdge(m_currBBp, elseBBp);
 
         // Build then branch
-        m_currBBp = &thenBB;
+        m_currBBp = thenBBp;
         iterateAndNextConstNull(nodep->thensp());
         if (!m_cfgp) return;
-        if (m_currBBp) addTakenEdge(*m_currBBp, contBB);
+        if (m_currBBp) m_cfgp->addTakenEdge(m_currBBp, contBBp);
 
         // Build else branch
-        m_currBBp = &elseBB;
+        m_currBBp = elseBBp;
         iterateAndNextConstNull(nodep->elsesp());
         if (!m_cfgp) return;
-        if (m_currBBp) addTakenEdge(*m_currBBp, contBB);
+        if (m_currBBp) m_cfgp->addTakenEdge(m_currBBp, contBBp);
 
         // Set continuation
-        m_currBBp = &contBB;
+        m_currBBp = contBBp;
     }
     void visit(AstWhile* nodep) override {
         if (!m_cfgp) return;
 
         // Create the header block
-        BasicBlock& headBB = addBasicBlock();
+        CfgBlock* const headBBp = m_cfgp->addBlock();
-        addTakenEdge(*m_currBBp, headBB);
+        m_cfgp->addTakenEdge(m_currBBp, headBBp);
 
         // The While goes in the header block - semantically the condition check only ...
-        m_currBBp = &headBB;
+        m_currBBp = headBBp;
         addStmt(nodep);
 
         // Create the body/continuation blocks
-        BasicBlock& bodyBB = addBasicBlock();
+        CfgBlock* const bodyBBp = m_cfgp->addBlock();
-        BasicBlock& contBB = addBasicBlock();
+        CfgBlock* const contBBp = m_cfgp->addBlock();
-        addTakenEdge(headBB, bodyBB);
+        m_cfgp->addTakenEdge(headBBp, bodyBBp);
-        addUntknEdge(headBB, contBB);
+        m_cfgp->addUntknEdge(headBBp, contBBp);
 
         // Build the body
-        m_currBBp = &bodyBB;
+        m_currBBp = bodyBBp;
         iterateAndNextConstNull(nodep->stmtsp());
         iterateAndNextConstNull(nodep->incsp());
         if (!m_cfgp) return;
-        if (m_currBBp) addTakenEdge(*m_currBBp, headBB);
+        if (m_currBBp) m_cfgp->addTakenEdge(m_currBBp, headBBp);
 
         // Set continuation
-        m_currBBp = &contBB;
+        m_currBBp = contBBp;
     }
     void visit(AstJumpBlock* nodep) override {
         if (!m_cfgp) return;
 
-        // Don't acutally need to add this 'nodep' to any block - but we could later if needed
+        // Don't acutally need to add this 'nodep' to any block
 
         // Create continuation block
-        BasicBlock& contBB = addBasicBlock();
+        CfgBlock* const contBBp = m_cfgp->addBlock();
-        const bool newEntry = m_jumpBlockContp.emplace(nodep, &contBB).second;
+        const bool newEntry = m_jumpBlockContp.emplace(nodep, contBBp).second;
         UASSERT_OBJ(newEntry, nodep, "AstJumpBlock visited twice");
 
         // Build the body
         iterateAndNextConstNull(nodep->stmtsp());
         if (!m_cfgp) return;
-        if (m_currBBp) addTakenEdge(*m_currBBp, contBB);
+        if (m_currBBp) m_cfgp->addTakenEdge(m_currBBp, contBBp);
 
         // Set continuation
-        m_currBBp = &contBB;
+        m_currBBp = contBBp;
     }
     void visit(AstJumpGo* nodep) override {
         if (!m_cfgp) return;
@@ -190,102 +173,61 @@ class CfgBuilder final : VNVisitorConst {
             return;
         }
 
-        // Don't acutally need to add this 'nodep' to any block - but we could later if needed
+        // Don't acutally need to add this 'nodep' to any block
 
         // Make current block go to the continuation of the JumpBlock
-        addTakenEdge(*m_currBBp, *m_jumpBlockContp.at(nodep->blockp()));
+        m_cfgp->addTakenEdge(m_currBBp, m_jumpBlockContp.at(nodep->blockp()));
 
         // There should be no statements after a JumpGo!
         m_currBBp = nullptr;
     }
 
     // CONSTRUCTOR
-    explicit CfgBuilder(const AstNodeProcedure* nodep) {
+    explicit CfgBuilder(AstNode* stmtsp) {
         // Build the graph, starting from the entry block
-        m_currBBp = &addBasicBlock();
+        m_currBBp = m_cfgp->addBlock();
         m_cfgp->m_enterp = m_currBBp;
         // Visit each statement to build the control flow graph
-        iterateAndNextConstNull(nodep->stmtsp());
+        iterateAndNextConstNull(stmtsp);
+        // If failed, stop now
         if (!m_cfgp) return;
         // The final block is the exit block
         m_cfgp->m_exitp = m_currBBp;
-
+        // Some blocks might not have predecessors if they are unreachable, remove them
-        // Remove empty blocks - except enter/exit
-        for (V3GraphVertex* const vtxp : m_cfgp->vertices().unlinkable()) {
-            if (vtxp == &m_cfgp->enter()) continue;
-            if (vtxp == &m_cfgp->exit()) continue;
-            BasicBlock* const bbp = vtxp->as<BasicBlock>();
-            if (!bbp->stmtps().empty()) continue;
-            UASSERT(bbp->outSize1(), "Empty block should have a single successor");
-            BasicBlock* const succp = const_cast<BasicBlock*>(bbp->takenSuccessorp());
-            for (V3GraphEdge* const edgep : bbp->inEdges().unlinkable()) edgep->relinkTop(succp);
-            vtxp->unlinkDelete(m_cfgp.get());
-        }
-        // Remove redundant entry block
-        while (m_cfgp->enter().stmtps().empty() && m_cfgp->enter().outSize1()) {
-            BasicBlock* const succp = m_cfgp->enter().outEdges().frontp()->top()->as<BasicBlock>();
-            if (!succp->inSize1()) break;
-            m_cfgp->m_enterp->unlinkDelete(m_cfgp.get());
-            m_cfgp->m_enterp = succp;
-        }
-        // Remove redundant exit block
-        while (m_cfgp->exit().stmtps().empty() && m_cfgp->exit().inSize1()) {
-            BasicBlock* const prep = m_cfgp->exit().inEdges().frontp()->fromp()->as<BasicBlock>();
-            if (!prep->outSize1()) break;
-            m_cfgp->m_exitp->unlinkDelete(m_cfgp.get());
-            m_cfgp->m_exitp = prep;
-        }
-
-        // Compute reverse post-order enumeration and sort blocks, assign IDs
         {
-            // Simple recursive algorith will do ...
+            std::vector<V3GraphVertex*> unreachableps;
-            std::vector<BasicBlock*> postOrderEnumeration;
+            for (V3GraphVertex* const vtxp : m_cfgp->vertices().unlinkable()) {
-            std::unordered_set<BasicBlock*> visited;
+                if (vtxp == m_cfgp->m_enterp) continue;
-            const std::function<void(BasicBlock*)> visitBasicBlock = [&](BasicBlock* bbp) {
+                if (vtxp == m_cfgp->m_exitp) continue;
-                // Mark and skip if already visited
+                UASSERT_OBJ(!vtxp->outEmpty(), vtxp, "Block with no successor other than exit");
-                if (!visited.emplace(bbp).second) return;
+                if (vtxp->inEmpty()) unreachableps.emplace_back(vtxp);
-                // Visit successors
+            }
-                for (const V3GraphEdge& e : bbp->outEdges()) {
+            while (!unreachableps.empty()) {
-                    visitBasicBlock(static_cast<BasicBlock*>(e.top()));
+                V3GraphVertex* const vtxp = unreachableps.back();
+                unreachableps.pop_back();
+                for (V3GraphEdge& edge : vtxp->outEdges()) {
+                    --m_cfgp->m_nEdges;
+                    if (edge.top()->inSize1()) unreachableps.emplace_back(edge.top());
                 }
-                // Add to post order enumeration
+                --m_cfgp->m_nBlocks;
-                postOrderEnumeration.emplace_back(bbp);
+                VL_DO_DANGLING(vtxp->unlinkDelete(m_cfgp.get()), vtxp);
-            };
-            visitBasicBlock(m_cfgp->m_enterp);
-            const uint32_t n = postOrderEnumeration.size();
-            UASSERT_OBJ(n == m_cfgp->vertices().size(), nodep, "Inconsistent enumeration size");
-
-            // Set size in graph
-            m_cfgp->m_nBasicBlocks = n;
-
-            // Assign ids equal to the reverse post order number and sort vertices
-            for (uint32_t i = 0; i < postOrderEnumeration.size(); ++i) {
-                BasicBlock* const bbp = postOrderEnumeration[n - 1 - i];
-                bbp->user(i);
-                m_cfgp->vertices().unlink(bbp);
-                m_cfgp->vertices().linkBack(bbp);
             }
         }
-
+        // Dump the initial graph
-        // Assign IDs to edges
+        if (dumpGraphLevel() >= 9) {
-        {
+            m_cfgp->rpoBlocks();
-            size_t nEdges = 0;
+            m_cfgp->dumpDotFilePrefixed("cfg-builder-initial");
-            for (V3GraphVertex& v : m_cfgp->vertices()) {
-                for (V3GraphEdge& e : v.outEdges()) e.user(nEdges++);
-            }
-            // Set size in graph
-            m_cfgp->m_nEdges = nEdges;
         }
-
+        // Minimize it
-        if (dumpGraphLevel() >= 9) m_cfgp->dumpDotFilePrefixed("cfgbuilder");
+        m_cfgp->minimize();
+        // Dump the final graph
+        if (dumpGraphLevel() >= 8) m_cfgp->dumpDotFilePrefixed("cfg-builder");
     }
 
 public:
-    static std::unique_ptr<ControlFlowGraph> apply(const AstNodeProcedure* nodep) {
+    static std::unique_ptr<CfgGraph> apply(AstNode* stmtsp) {
-        return std::move(CfgBuilder{nodep}.m_cfgp);
+        return std::move(CfgBuilder{stmtsp}.m_cfgp);
     }
 };
 
-std::unique_ptr<const ControlFlowGraph> V3Cfg::build(const AstNodeProcedure* nodep) {
+std::unique_ptr<CfgGraph> CfgGraph::build(AstNode* stmtsp) { return CfgBuilder::apply(stmtsp); }
-    return CfgBuilder::apply(nodep);
-}

src/V3CfgLiveVariables.cpp

@@ -49,9 +49,9 @@ class CfgLiveVariables final : VNVisitorConst {
     };
 
     // STATE
-    const ControlFlowGraph& m_cfg;  // The CFG beign analysed
+    const CfgGraph& m_cfg;  // The CFG beign analysed
     // State for each block
-    BasicBlockMap<BlockState> m_blockState = m_cfg.makeBasicBlockMap<BlockState>();
+    CfgBlockMap<BlockState> m_blockState{m_cfg.makeBlockMap<BlockState>()};
     BlockState* m_currp = nullptr;  // State of current block being analysed
     bool m_abort = false;  // Abort analysis - unhandled construct
 
@@ -140,7 +140,7 @@ class CfgLiveVariables final : VNVisitorConst {
     }
 
     // Apply transfer function of block, return true if changed
-    bool transfer(const BasicBlock& bb) {
+    bool transfer(const CfgBlock& bb) {
         BlockState& state = m_blockState[bb];
 
         // liveIn = gen union (liveOut - kill)
@@ -172,20 +172,21 @@ class CfgLiveVariables final : VNVisitorConst {
     void visit(AstWhile* nodep) override { single(nodep->condp()); }
 
     // CONSTRUCTOR
-    explicit CfgLiveVariables(const ControlFlowGraph& cfg)
+    explicit CfgLiveVariables(const CfgGraph& cfg)
         : m_cfg{cfg} {
         // For each basic block, compute the gen and kill set via visit
-        cfg.foreach([&](const BasicBlock& bb) {
+        for (const V3GraphVertex& vtx : cfg.vertices()) {
+            const CfgBlock& bb = static_cast<const CfgBlock&>(vtx);
             if (m_abort) return;
             VL_RESTORER(m_currp);
             m_currp = &m_blockState[bb];
             for (AstNode* const stmtp : bb.stmtps()) iterateConst(stmtp);
-        });
+        }
         if (m_abort) return;
 
         // Perform the flow analysis
-        std::deque<const BasicBlock*> workList;
+        std::deque<const CfgBlock*> workList;
-        const auto enqueue = [&](const BasicBlock& bb) {
+        const auto enqueue = [&](const CfgBlock& bb) {
             BlockState& state = m_blockState[bb];
             if (state.m_isOnWorkList) return;
             state.m_isOnWorkList = true;
@@ -195,13 +196,13 @@ class CfgLiveVariables final : VNVisitorConst {
         enqueue(cfg.exit());
 
         while (!workList.empty()) {
-            const BasicBlock* const currp = workList.front();
+            const CfgBlock* const currp = workList.front();
             workList.pop_front();
             BlockState& state = m_blockState[*currp];
             state.m_isOnWorkList = false;
 
             // Compute meet (liveOut = union liveIn of successors)
-            currp->forEachSuccessor([&](const BasicBlock& bb) {
+            currp->forEachSuccessor([&](const CfgBlock& bb) {
                 auto& liveIn = m_blockState[bb].m_liveIn;
                 state.m_liveOut.insert(liveIn.begin(), liveIn.end());
             });
@@ -216,7 +217,7 @@ class CfgLiveVariables final : VNVisitorConst {
     }
 
 public:
-    static std::unique_ptr<std::vector<Variable*>> apply(const ControlFlowGraph& cfg) {
+    static std::unique_ptr<std::vector<Variable*>> apply(const CfgGraph& cfg) {
         CfgLiveVariables analysis{cfg};
         // If failed, return nullptr
         if (analysis.m_abort) return nullptr;
@@ -231,10 +232,10 @@ public:
     }
 };
 
-std::unique_ptr<std::vector<AstVar*>> V3Cfg::liveVars(const ControlFlowGraph& cfg) {
+std::unique_ptr<std::vector<AstVar*>> V3Cfg::liveVars(const CfgGraph& cfg) {
     return CfgLiveVariables</* T_Scoped: */ false>::apply(cfg);
 }
 
-std::unique_ptr<std::vector<AstVarScope*>> V3Cfg::liveVarScopes(const ControlFlowGraph& cfg) {
+std::unique_ptr<std::vector<AstVarScope*>> V3Cfg::liveVarScopes(const CfgGraph& cfg) {
     return CfgLiveVariables</* T_Scoped: */ true>::apply(cfg);
 }

src/V3DfgAstToDfg.cpp

@@ -57,7 +57,7 @@ class AstToDfgVisitor final : public VNVisitor {
         }
     }
 
-    std::unique_ptr<std::vector<Variable*>> getLiveVariables(const ControlFlowGraph& cfg) {
+    std::unique_ptr<std::vector<Variable*>> getLiveVariables(const CfgGraph& cfg) {
         // TODO: remove the useless reinterpret_casts when C++17 'if constexpr' actually works
         if VL_CONSTEXPR_CXX17 (T_Scoped) {
             std::unique_ptr<std::vector<AstVarScope*>> result = V3Cfg::liveVarScopes(cfg);
@@ -139,7 +139,7 @@ class AstToDfgVisitor final : public VNVisitor {
 
     // Gather variables live in to the given CFG.
     // Return nullptr if any are not supported.
-    std::unique_ptr<std::vector<DfgVertexVar*>> gatherLive(const ControlFlowGraph& cfg) {
+    std::unique_ptr<std::vector<DfgVertexVar*>> gatherLive(const CfgGraph& cfg) {
         // Run analysis
         std::unique_ptr<std::vector<Variable*>> varps = getLiveVariables(cfg);
         if (!varps) {
@@ -206,7 +206,7 @@ class AstToDfgVisitor final : public VNVisitor {
         // Potentially convertible block
         ++m_ctx.m_inputs;
         // Attempt to build CFG of AstAlways, give up if failed
-        std::unique_ptr<const ControlFlowGraph> cfgp = V3Cfg::build(nodep);
+        std::unique_ptr<CfgGraph> cfgp = CfgGraph::build(nodep->stmtsp());
         if (!cfgp) {
             ++m_ctx.m_nonRepCfg;
             return false;

src/V3DfgContext.h

@@ -251,7 +251,13 @@ public:
         // Reverted
         VDouble0 revertNonSyn;  // Reverted due to being driven from non-synthesizable vertex
         VDouble0 revertMultidrive;  // Reverted due to multiple drivers
-
+        // Additional stats
+        VDouble0 cfgTrivial;  // Trivial input CFGs
+        VDouble0 cfgSp;  // Series-paralel input CFGs
+        VDouble0 cfgDag;  // Generic loop free input CFGs
+        VDouble0 cfgCyclic;  // Cyclic input CFGs
+        VDouble0 joinUsingPathPredicate;  // Num control flow joins using full path predicate
+        VDouble0 joinUsingBranchCondition;  // Num Control flow joins using dominating branch cond
     } m_synt;
 
 private:
@@ -314,6 +320,14 @@ private:
         nSyntExpect -= m_synt.synthAlways;
         nSyntExpect -= m_synt.synthAssign;
         UASSERT(nSyntNonSyn == nSyntExpect, "Inconsistent statistics / synt");
+
+        addStat("synt / input CFG trivial", m_synt.cfgTrivial);
+        addStat("synt / input CFG sp", m_synt.cfgSp);
+        addStat("synt / input CFG dag", m_synt.cfgDag);
+        addStat("synt / input CFG cyclic", m_synt.cfgCyclic);
+
+        addStat("synt / joins using path predicate", m_synt.joinUsingPathPredicate);
+        addStat("synt / joins using branch condition", m_synt.joinUsingBranchCondition);
     }
 };
 

src/V3DfgSynthesize.cpp

@@ -508,15 +508,23 @@ class AstToDfgSynthesize final {
         bool operator<=(const Driver& other) const { return !(other < *this); }
     };
 
-    // STATE
+    // STATE - Persistent
     DfgGraph& m_dfg;  // The graph being built
     V3DfgSynthesisContext& m_ctx;  // The context for stats
     AstToDfgConverter<T_Scoped> m_converter;  // The convert instance to use for each construct
-    DfgLogic* m_logicp = nullptr;  // Current logic vertex we are synthesizing
 
-    // Some debug aid: We stop after synthesizing s_dfgSynthDebugLimit vertices (if non-zero).
+    // STATE - for current DfgLogic being synthesized
-    // This is the problematic logic (last one we synthesize), assuming a bisection search
+    DfgLogic* m_logicp = nullptr;  // Current logic vertex we are synthesizing
-    // over s_dfgSynthDebugLimit.
+    CfgBlockMap<SymTab> m_bbToISymTab;  // Map from CfgBlock -> input symbol table
+    CfgBlockMap<SymTab> m_bbToOSymTab;  // Map from CfgBlock -> output symbol table
+    CfgBlockMap<DfgVertex*> m_bbToCondp;  // Map from CfgBlock ->  terminating branch condition
+    CfgEdgeMap<DfgVertex*> m_edgeToPredicatep;  // Map CfgGraphEdge -> path predicate to get there
+    CfgDominatorTree m_domTree;  // The dominator tree of the current CFG
+
+    // STATE - Some debug aid
+    // We stop after synthesizing s_dfgSynthDebugLimit vertices (if non-zero).
+    // This is the problematic logic (last one we synthesize), assuming a
+    // bisection search over s_dfgSynthDebugLimit.
     DfgLogic* m_debugLogicp = nullptr;
     // Source (upstream) cone of outputs of m_debugLogicp
     std::unique_ptr<std::unordered_set<const DfgVertex*>> m_debugOSrcConep{nullptr};
@@ -869,7 +877,7 @@ class AstToDfgSynthesize final {
         });
     }
 
-    // Initialzie input symbol table of entry BasicBlock
+    // Initialzie input symbol table of entry CfgBlock
     void initializeEntrySymbolTable(SymTab& iSymTab) {
         m_logicp->forEachSource([&](DfgVertex& src) {
             DfgVertexVar* const vvp = src.as<DfgVertexVar>();
@@ -991,75 +999,152 @@ class AstToDfgSynthesize final {
         return joinp;
     }
 
+    // Merge 'thenSymTab' into 'elseSymTab' using the given predicate to join values
+    bool joinSymbolTables(SymTab& elseSymTab, DfgVertex* predicatep, const SymTab& thenSymTab) {
+        // Give up if something is not assigned on all paths ... Latch?
+        if (thenSymTab.size() != elseSymTab.size()) {
+            ++m_ctx.m_synt.nonSynLatch;
+            return false;
+        }
+        // Join each symbol
+        for (std::pair<Variable* const, DfgVertexVar*>& pair : elseSymTab) {
+            Variable* const varp = pair.first;
+            // Find same variable on the else path
+            auto it = thenSymTab.find(varp);
+            // Give up if something is not assigned on all paths ... Latch?
+            if (it == thenSymTab.end()) {
+                ++m_ctx.m_synt.nonSynLatch;
+                return false;
+            }
+            // Join paths with the predicate
+            DfgVertexVar* const thenp = it->second;
+            DfgVertexVar* const elsep = pair.second;
+            DfgVertexVar* const newp = joinDrivers(varp, predicatep, thenp, elsep);
+            if (!newp) return false;
+            pair.second = newp;
+        }
+        // Done
+        return true;
+    }
+
+    // Given two joining control flow edges, compute how to join their symbols.
+    // Returns the predicaete to join over, and the 'then' and 'else' blocks.
+    std::tuple<DfgVertex*, const CfgBlock*, const CfgBlock*>  //
+    howToJoin(const CfgEdge* const ap, const CfgEdge* const bp) {
+        // Find the closest common dominator of the two paths
+        const CfgBlock* const domp = m_domTree.closestCommonDominator(ap->srcp(), bp->srcp());
+        // These paths join here, so 'domp' must be a branch, otherwise it's not the closest
+        UASSERT_OBJ(domp->isBranch(), domp, "closestCommonDominator is not a branch");
+
+        // The branches of the common dominator
+        const CfgEdge* const takenEdgep = domp->takenEdgep();
+        const CfgEdge* const untknEdgep = domp->untknEdgep();
+
+        // We check if the taken branch dominates the path to either blocks,
+        // and if the untaken branch dominates the path to the other block.
+        // If so, we can use the branch condition as predicate, otherwise
+        // we must use the path predicate as there are ways to get from one
+        // branch of the dominator to the other. We need to be careful if
+        // either branches are directly to the join block. This is fine,
+        // it's as if there was an empty block on that critical edge which
+        // is dominated by that path.
+
+        if (takenEdgep == ap || m_domTree.dominates(takenEdgep->dstp(), ap->srcp())) {
+            if (untknEdgep == bp || m_domTree.dominates(untknEdgep->dstp(), bp->srcp())) {
+                // Taken path dominates 'ap' and untaken dominates 'bp', use the branch condition
+                ++m_ctx.m_synt.joinUsingBranchCondition;
+                return std::make_tuple(m_bbToCondp[domp], ap->srcp(), bp->srcp());
+            }
+        } else if (takenEdgep == bp || m_domTree.dominates(takenEdgep->dstp(), bp->srcp())) {
+            if (untknEdgep == ap || m_domTree.dominates(untknEdgep->dstp(), ap->srcp())) {
+                // Taken path dominates 'bp' and untaken dominates 'ap', use the branch condition
+                ++m_ctx.m_synt.joinUsingBranchCondition;
+                return std::make_tuple(m_bbToCondp[domp], bp->srcp(), ap->srcp());
+            }
+        }
+
+        // The branches don't dominate the joined blocks, must use the path predicate
+        ++m_ctx.m_synt.joinUsingPathPredicate;
+
+        // TODO: We could do better here: use the path predicate of the closest
+        // cominating blocks, pick the one from the lower rank, etc, but this
+        // generic case is very rare, most synthesizable logic has
+        // series-parallel CFGs which are covered by the earlier cases.
+        return std::make_tuple(m_edgeToPredicatep[ap], ap->srcp(), bp->srcp());
+    }
+
     // Combine the output symbol tables of the predecessors of the given
-    // BasicBlock to compute the input symtol table for the given block.
+    // block to compute the input symtol table for the given block.
-    bool createInputSymbolTable(SymTab& joined, const BasicBlock& bb,
+    bool createInputSymbolTable(const CfgBlock& bb) {
-                                const BasicBlockMap<SymTab>& bbToOSymTab,
+        // The input symbol table of the given block, we are computing it now
-                                const ControlFlowEdgeMap<DfgVertex*>& edgeToPredicatep) {
+        SymTab& joined = m_bbToISymTab[bb];
-        // Input symbol table of entry block was previously initialzied
-        if (bb.inEmpty()) return true;
 
-        // We will fill it in here
+        // Input symbol table of entry block is special
-        UASSERT(joined.empty(), "Unresolved input symbol table should be empty");
+        if (bb.isEnter()) {
-
+            initializeEntrySymbolTable(joined);
-        // Fast path if there is only one predecessor
-        if (bb.inSize1()) {
-            joined = bbToOSymTab[*(bb.inEdges().frontp()->fromp()->as<BasicBlock>())];
             return true;
         }
 
-        // Gather predecessors and the path predicates
+        // Current input symbol table should be empty, we will fill it in here
+        UASSERT(joined.empty(), "Unprocessed input symbol table should be empty");
+
+        // Fast path if there is only one predecessor - TODO: use less copying
+        if (!bb.isJoin()) {
+            joined = m_bbToOSymTab[bb.firstPredecessorp()];
+            return true;
+        }
+
+        // We also have a simpler job if there are 2 predecessors
+        if (bb.isTwoWayJoin()) {
+            DfgVertex* predicatep = nullptr;
+            const CfgBlock* thenp = nullptr;
+            const CfgBlock* elsep = nullptr;
+            std::tie(predicatep, thenp, elsep)
+                = howToJoin(bb.firstPredecessorEdgep(), bb.lastPredecessorEdgep());
+            // Copy from else
+            joined = m_bbToOSymTab[elsep];
+            // Join with then
+            return joinSymbolTables(joined, predicatep, m_bbToOSymTab[*thenp]);
+        }
+
+        // General hard way
+
+        // Gather predecessors
         struct Predecessor final {
-            const BasicBlock* m_bbp;
+            const CfgBlock* m_bbp;  // Predeccessor block
-            DfgVertex* m_predicatep;
+            DfgVertex* m_predicatep;  // Predicate predecessor reached this block with
+            const SymTab* m_oSymTabp;  // Output symbol table or predecessor
             Predecessor() = delete;
-            Predecessor(const BasicBlock* bbp, DfgVertex* predicatep)
+            Predecessor(const CfgBlock* bbp, DfgVertex* predicatep, const SymTab* oSymTabp)
                 : m_bbp{bbp}
-                , m_predicatep{predicatep} {}
+                , m_predicatep{predicatep}
+                , m_oSymTabp{oSymTabp} {}
         };
 
         const std::vector<Predecessor> predecessors = [&]() {
             std::vector<Predecessor> res;
             for (const V3GraphEdge& edge : bb.inEdges()) {
-                const ControlFlowEdge& cfgEdge = static_cast<const ControlFlowEdge&>(edge);
+                const CfgEdge& cfgEdge = static_cast<const CfgEdge&>(edge);
-                res.emplace_back(&cfgEdge.src(), edgeToPredicatep[cfgEdge]);
+                const CfgBlock* const predecessorp = cfgEdge.srcp();
+                DfgVertex* const predicatep = m_edgeToPredicatep[cfgEdge];
+                const SymTab* const oSymTabp = &m_bbToOSymTab[predecessorp];
+                res.emplace_back(predecessorp, predicatep, oSymTabp);
             }
-            // Sort predecessors topologically. This way later blocks will come
+            // Sort predecessors reverse topologically. This way earlier blocks
-            // after earlier blocks, and the entry block will be first if present.
+            // will come after later blocks, and the entry block is last if present.
             std::sort(res.begin(), res.end(), [](const Predecessor& a, const Predecessor& b) {  //
-                return a.m_bbp->id() < b.m_bbp->id();
+                return *a.m_bbp > *b.m_bbp;
             });
             return res;
         }();
 
-        // Start by copying the bindings from the oldest predecessor
+        // Start by copying the bindings from the frist predecessor
-        joined = bbToOSymTab[*predecessors[0].m_bbp];
+        joined = *predecessors[0].m_oSymTabp;
         // Join over all other predecessors
         for (size_t i = 1; i < predecessors.size(); ++i) {
             DfgVertex* const predicatep = predecessors[i].m_predicatep;
-            const SymTab& oSymTab = bbToOSymTab[*predecessors[i].m_bbp];
+            const SymTab& oSymTab = *predecessors[i].m_oSymTabp;
-            // Give up if something is not assigned on all paths ... Latch?
+            if (!joinSymbolTables(joined, predicatep, oSymTab)) return false;
-            if (joined.size() != oSymTab.size()) {
-                ++m_ctx.m_synt.nonSynLatch;
-                return false;
-            }
-            // Join each symbol
-            for (auto& pair : joined) {
-                Variable* const varp = pair.first;
-                // Find same variable on other path
-                auto it = oSymTab.find(varp);
-                // Give up if something is not assigned on all paths ... Latch?
-                if (it == oSymTab.end()) {
-                    ++m_ctx.m_synt.nonSynLatch;
-                    return false;
-                }
-                // Join paths with the block predicate
-                DfgVertexVar* const thenp = it->second;
-                DfgVertexVar* const elsep = pair.second;
-                DfgVertexVar* const newp = joinDrivers(varp, predicatep, thenp, elsep);
-                if (!newp) return false;
-                pair.second = newp;
-            }
         }
 
         return true;
@@ -1169,7 +1254,7 @@ class AstToDfgSynthesize final {
     }
 
     // Synthesize the given statements with the given input symbol table.
-    // Returnt true if successfolly synthesized.
+    // Returns true if successfolly synthesized.
     // Populates the given output symbol table.
     // Populates the given reference with the condition of the terminator branch, if any.
     bool synthesizeBasicBlock(SymTab& oSymTab, DfgVertex*& condpr,
@@ -1259,56 +1344,53 @@ class AstToDfgSynthesize final {
         return true;
     }
 
-    // Given a basic block, and the condition of the terminating branch (if any),
+    // Assign path perdicates to the outgoing control flow edges of the given block
-    // assign perdicates to the block's outgoing control flow edges.
+    void assignPathPredicates(const CfgBlock& bb) {
-    void assignSuccessorPredicates(ControlFlowEdgeMap<DfgVertex*>& edgeToPredicatep,
-                                   const BasicBlock& bb, DfgVertex* condp) {
         // Nothing to do for the exit block
-        if (bb.outEmpty()) return;
+        if (bb.isExit()) return;
 
         // Get the predicate of this block
         DfgVertex* const predp = [&]() -> DfgVertex* {
             // Entry block has no predecessors, use constant true
-            if (bb.inEmpty()) return make<DfgConst>(m_logicp->fileline(), 1U, 1U);
+            if (bb.isEnter()) return make<DfgConst>(m_logicp->fileline(), 1U, 1U);
 
             // For any other block, 'or' together all the incoming predicates
             const auto& inEdges = bb.inEdges();
             auto it = inEdges.begin();
-            DfgVertex* resp = edgeToPredicatep[static_cast<const ControlFlowEdge&>(*it)];
+            DfgVertex* resp = m_edgeToPredicatep[static_cast<const CfgEdge&>(*it)];
             while (++it != inEdges.end()) {
                 DfgOr* const orp = make<DfgOr>(resp->fileline(), resp->dtypep());
                 orp->rhsp(resp);
-                orp->lhsp(edgeToPredicatep[static_cast<const ControlFlowEdge&>(*it)]);
+                orp->lhsp(m_edgeToPredicatep[static_cast<const CfgEdge&>(*it)]);
                 resp = orp;
             }
             return resp;
         }();
 
-        if (!condp) {
+        // For uncondional branches, the successor predicate edge is the same
-            // There should be 1 successors for a block with an unconditional terminator
+        if (!bb.isBranch()) {
-            UASSERT_OBJ(!bb.untknEdgep(), predp, "Expecting 1 successor for BasicBlock");
+            m_edgeToPredicatep[bb.takenEdgep()] = predp;
-            // Successor predicate edge is the same
+            return;
-            edgeToPredicatep[*bb.takenEdgep()] = predp;
-        } else {
-            // There should be 2 successors for a block with an conditional terminator
-            UASSERT_OBJ(bb.untknEdgep(), predp, "Expecting 2 successors for BasicBlock");
-            FileLine* const flp = condp->fileline();
-            AstNodeDType* const dtypep = condp->dtypep();  // Single bit
-
-            // Predicate for taken branch: 'predp & condp'
-            DfgAnd* const takenPredp = make<DfgAnd>(flp, dtypep);
-            takenPredp->lhsp(predp);
-            takenPredp->rhsp(condp);
-            edgeToPredicatep[*bb.takenEdgep()] = takenPredp;
-
-            // Predicate for untaken branch: 'predp & ~condp'
-            DfgAnd* const untknPredp = make<DfgAnd>(flp, dtypep);
-            untknPredp->lhsp(predp);
-            DfgNot* const notp = make<DfgNot>(flp, dtypep);
-            notp->srcp(condp);
-            untknPredp->rhsp(notp);
-            edgeToPredicatep[*bb.untknEdgep()] = untknPredp;
         }
+
+        // For branches, we need to factor in the branch condition
+        DfgVertex* const condp = m_bbToCondp[bb];
+        FileLine* const flp = condp->fileline();
+        AstNodeDType* const dtypep = condp->dtypep();  // Single bit
+
+        // Predicate for taken branch: 'predp & condp'
+        DfgAnd* const takenPredp = make<DfgAnd>(flp, dtypep);
+        takenPredp->lhsp(predp);
+        takenPredp->rhsp(condp);
+        m_edgeToPredicatep[bb.takenEdgep()] = takenPredp;
+
+        // Predicate for untaken branch: 'predp & ~condp'
+        DfgAnd* const untknPredp = make<DfgAnd>(flp, dtypep);
+        untknPredp->lhsp(predp);
+        DfgNot* const notp = make<DfgNot>(flp, dtypep);
+        notp->srcp(condp);
+        untknPredp->rhsp(notp);
+        m_edgeToPredicatep[bb.untknEdgep()] = untknPredp;
     }
 
     // Add the synthesized values as drivers to the output variables of the current DfgLogic
@@ -1359,7 +1441,7 @@ class AstToDfgSynthesize final {
         // Initialzie input symbol table
         initializeEntrySymbolTable(iSymTab);
 
-        // Synthesize as if it was in a single BasicBlock CFG
+        // Synthesize as if it was in a single CfgBlock CFG
         DfgVertex* condp = nullptr;
         const bool success = synthesizeBasicBlock(oSymTab, condp, {assignp}, iSymTab);
         UASSERT_OBJ(!condp, nodep, "Conditional AstAssignW ???");
@@ -1372,7 +1454,7 @@ class AstToDfgSynthesize final {
     }
 
     // Synthesize the given AstAlways. Returns true on success.
-    bool synthesizeCfg(const ControlFlowGraph& cfg) {
+    bool synthesizeCfg(CfgGraph& cfg) {
         ++m_ctx.m_synt.inputAlways;
 
         if (hasExternallyWrittenVariable(*m_logicp)) {
@@ -1383,37 +1465,48 @@ class AstToDfgSynthesize final {
         // If there is a backward edge (loop), we can't synthesize it
         if (cfg.containsLoop()) {
             ++m_ctx.m_synt.nonSynLoop;
+            ++m_ctx.m_synt.cfgCyclic;
             return false;
         }
 
-        // Maps from BasicBlock to its input and output symbol tables
+        // If it's a trivial CFG we can save on some work
-        BasicBlockMap<SymTab> bbToISymTab = cfg.makeBasicBlockMap<SymTab>();
+        if (cfg.nBlocks() == 1) {
-        BasicBlockMap<SymTab> bbToOSymTab = cfg.makeBasicBlockMap<SymTab>();
+            ++m_ctx.m_synt.cfgTrivial;
+        } else {
+            // Insert two-way join blocks to aid multiplexer ordering
+            if (cfg.insertTwoWayJoins()) {
+                ++m_ctx.m_synt.cfgSp;
+            } else {
+                ++m_ctx.m_synt.cfgDag;
+            }
+            // Initialize maps needed for non-trivial CFGs
+            m_domTree = CfgDominatorTree{cfg};
+            m_edgeToPredicatep = cfg.makeEdgeMap<DfgVertex*>();
+        }
 
-        // Map from ControlFlowGraphEdge to its predicate
+        // Initialize CfgMaps
-        ControlFlowEdgeMap<DfgVertex*> edgeToPredicatep = cfg.makeEdgeMap<DfgVertex*>();
+        m_bbToISymTab = cfg.makeBlockMap<SymTab>();
-
+        m_bbToOSymTab = cfg.makeBlockMap<SymTab>();
-        // Initialzie input symbol table of entry block
+        m_bbToCondp = cfg.makeBlockMap<DfgVertex*>();
-        initializeEntrySymbolTable(bbToISymTab[cfg.enter()]);
 
         // Synthesize all blocks
-        for (const V3GraphVertex& cfgVtx : cfg.vertices()) {
+        for (const V3GraphVertex& vtx : cfg.vertices()) {
-            const BasicBlock& bb = *cfgVtx.as<BasicBlock>();
+            const CfgBlock& bb = static_cast<const CfgBlock&>(vtx);
-            // Symbol tables of the block
+            // Prepare the input symbol table of this block (enter, or join predecessor blocks)
-            SymTab& iSymTab = bbToISymTab[bb];
+            if (!createInputSymbolTable(bb)) return false;
-            SymTab& oSymTab = bbToOSymTab[bb];
+            // Synthesize this block
-            // Join symbol tables from predecessor blocks
+            if (!synthesizeBasicBlock(m_bbToOSymTab[bb],  //
-            if (!createInputSymbolTable(iSymTab, bb, bbToOSymTab, edgeToPredicatep)) return false;
+                                      m_bbToCondp[bb],  //
-            // Condition of the terminating branch, if any
+                                      bb.stmtps(),  //
-            DfgVertex* condp = nullptr;
+                                      m_bbToISymTab[bb])) {
-            // Synthesize the block
+                return false;
-            if (!synthesizeBasicBlock(oSymTab, condp, bb.stmtps(), iSymTab)) return false;
+            }
-            // Set the predicates on the successor edges
+            // Set the path predicates on the successor edges
-            assignSuccessorPredicates(edgeToPredicatep, bb, condp);
+            assignPathPredicates(bb);
         }
 
         // Add resolved output variable drivers
-        return addSynthesizedOutput(bbToOSymTab[cfg.exit()]);
+        return addSynthesizedOutput(m_bbToOSymTab[cfg.exit()]);
     }
 
     // Synthesize a DfgLogic into regular vertices. Returns ture on success.
@@ -1676,7 +1769,7 @@ void V3DfgPasses::synthesize(DfgGraph& dfg, V3DfgContext& ctx) {
                 if (VN_IS(logicp->nodep(), AssignW)) return true;
                 // Synthesize always blocks with no more than 4 basic blocks and 4 edges
                 // These are usually simple branches (if (rst) ... else ...), or close to it
-                return logicp->cfg().nBasicBlocks() <= 4 && logicp->cfg().nEdges() <= 4;
+                return logicp->cfg().nBlocks() <= 4 && logicp->cfg().nEdges() <= 4;
             });
             if (doIt) varps.emplace_back(&var);
         }

src/V3DfgVertices.h

@@ -346,7 +346,7 @@ public:
 class DfgLogic final : public DfgVertexVariadic {
     // Generic vertex representing a whole combinational process
     AstNode* const m_nodep;  // The Ast logic represented by this vertex
-    const std::unique_ptr<const ControlFlowGraph> m_cfgp;
+    const std::unique_ptr<CfgGraph> m_cfgp;
     // Vertices this logic was synthesized into. Excluding variables
     std::vector<DfgVertex*> m_synth;
 
@@ -356,7 +356,7 @@ public:
         , m_nodep{nodep}
         , m_cfgp{nullptr} {}
 
-    DfgLogic(DfgGraph& dfg, AstAlways* nodep, std::unique_ptr<const ControlFlowGraph> cfgp)
+    DfgLogic(DfgGraph& dfg, AstAlways* nodep, std::unique_ptr<CfgGraph> cfgp)
         : DfgVertexVariadic{dfg, dfgType(), nodep->fileline(), nullptr, 1u}
         , m_nodep{nodep}
         , m_cfgp{std::move(cfgp)} {}
@@ -366,7 +366,8 @@ public:
     void addInput(DfgVertexVar* varp) { addSource()->relinkSource(varp); }
 
     AstNode* nodep() const { return m_nodep; }
-    const ControlFlowGraph& cfg() const { return *m_cfgp; }
+    CfgGraph& cfg() { return *m_cfgp; }
+    const CfgGraph& cfg() const { return *m_cfgp; }
     std::vector<DfgVertex*>& synth() { return m_synth; }
     const std::vector<DfgVertex*>& synth() const { return m_synth; }
 

src/V3List.h

@@ -241,6 +241,7 @@ public:
     bool empty() const { return !m_headp; }
     bool hasSingleElement() const { return m_headp && m_headp == m_lastp; }
     bool hasMultipleElements() const { return m_headp && m_headp != m_lastp; }
+    bool hasTwoElements() const { return m_headp && toLinks(m_headp).m_nextp == m_lastp; }
 
     // These return pointers, as we often want to unlink/delete them, and can also signal empty
     T_Element* frontp() { return static_cast<T_Element*>(m_headp); }

test_regress/t/t_dfg_break_cycles.py

@@ -44,9 +44,10 @@ with open(rdFile, 'r', encoding="utf8") as rdFh, \
      open(pdeclFile, 'w', encoding="utf8") as pdeclFh, \
      open(checkFile, 'w', encoding="utf8") as checkFh:
     for line in rdFh:
-        if "// UNOPTFLAT" in line:
+        line, _, cmt = line.partition("//")
+        cmt, _, _ = cmt.partition("//")
+        if "UNOPTFLAT" in cmt:
             nExpectedCycles += 1
-        line = line.split("//")[0]
         m = re.search(r'`signal\((\w+),', line)
         if not m:
             continue

test_regress/t/t_dfg_synthesis.py

@@ -70,13 +70,14 @@ test.compile(verilator_flags2=[
     "--stats",
     "--build",
     "--fdfg-synthesize-all",
+    "-fno-dfg-pre-inline",
     "-fno-dfg-post-inline",
-    "-fno-dfg-scoped",
     "--exe",
     "+incdir+" + test.obj_dir,
     "-Mdir", test.obj_dir + "/obj_opt",
     "--prefix", "Vopt",
     "-fno-const-before-dfg",  # Otherwise V3Const makes testing painful
+    "-fno-split", # Dfg will take care of it
     "--debug", "--debugi", "0", "--dumpi-tree", "0",
     "-CFLAGS \"-I .. -I ../obj_ref\"",
     "../obj_ref/Vref__ALL.a",
@@ -84,14 +85,13 @@ test.compile(verilator_flags2=[
 ])  # yapf:disable
 
 test.file_grep(test.obj_dir + "/obj_opt/Vopt__stats.txt",
-               r'DFG pre inline Synthesis, synt / always blocks considered\s+(\d+)$',
+               r'DFG scoped Synthesis, synt / always blocks considered\s+(\d+)$',
                nAlwaysSynthesized + nAlwaysReverted + nAlwaysNotSynthesized)
 test.file_grep(test.obj_dir + "/obj_opt/Vopt__stats.txt",
-               r'DFG pre inline Synthesis, synt / always blocks synthesized\s+(\d+)$',
+               r'DFG scoped Synthesis, synt / always blocks synthesized\s+(\d+)$',
                nAlwaysSynthesized + nAlwaysReverted)
 test.file_grep(test.obj_dir + "/obj_opt/Vopt__stats.txt",
-               r'DFG pre inline Synthesis, synt / reverted \(multidrive\)\s+(\d)$',
+               r'DFG scoped Synthesis, synt / reverted \(multidrive\)\s+(\d)$', nAlwaysReverted)
-               nAlwaysReverted)
 
 # Execute test to check equivalence
 test.execute(executable=test.obj_dir + "/obj_opt/Vopt")

test_regress/t/t_dfg_synthesis.v

@@ -154,6 +154,176 @@ module t (
   end
   `signal(CONDITONAL_F, condigional_f);
 
+  logic [2:0] conditional_g;
+  always_comb begin
+    if (rand_a[0]) begin
+      if (rand_a[1]) begin
+        if (rand_a[2]) begin
+          conditional_g = 3'b111;
+        end else begin
+          conditional_g = 3'b011;
+        end
+      end else begin
+        if (rand_a[2]) begin
+          conditional_g = 3'b101;
+        end else begin
+          conditional_g = 3'b001;
+        end
+      end
+    end else begin
+      if (rand_a[1]) begin
+        if (rand_a[2]) begin
+          conditional_g = 3'b110;
+        end else begin
+          conditional_g = 3'b010;
+        end
+      end else begin
+        if (rand_a[2]) begin
+          conditional_g = 3'b100;
+        end else begin
+          conditional_g = 3'b000;
+        end
+      end
+    end
+  end
+  `signal(CONDITONAL_G, conditional_g);
+
+  logic [2:0] conditional_h;
+  always_comb begin
+    if (rand_a[0]) begin
+      if (rand_a[1]) begin
+        conditional_h = 3'b011;
+        if (rand_a[2]) begin
+          conditional_h = 3'b111;
+        end
+      end else begin
+        conditional_h = 3'b001;
+        if (rand_a[2]) begin
+          conditional_h = 3'b101;
+        end
+      end
+    end else begin
+      if (rand_a[1]) begin
+        conditional_h = 3'b010;
+        if (rand_a[2]) begin
+          conditional_h = 3'b110;
+        end
+      end else begin
+        conditional_h = 3'b000;
+        if (rand_a[2]) begin
+          conditional_h = 3'b100;
+        end
+      end
+    end
+  end
+  `signal(CONDITONAL_H, conditional_h);
+
+  logic [2:0] conditional_i;
+  always_comb begin // Dumbass trailing zeroes count
+    do begin
+      conditional_i = 3'd0;
+      if (rand_a[0]) break;
+      conditional_i = 3'd1;
+      if (rand_a[1]) break;
+      conditional_i = 3'd2;
+      if (rand_a[2]) break;
+      conditional_i = 3'd3;
+      if (rand_a[3]) break;
+      conditional_i = 3'd4;
+    end while (0);
+  end
+  `signal(CONDITONAL_I, conditional_i);
+
+  logic [2:0] conditional_j;
+  always_comb begin // Even more dumbass trailing ones count
+    do begin
+      conditional_j = 3'd0;
+      if (rand_a[0]) begin
+        conditional_j = 3'd1;
+      end else begin
+        break;
+      end
+      if (rand_a[1]) begin
+        conditional_j = 3'd2;
+      end else begin
+        break;
+      end
+      if (rand_a[2]) begin
+        conditional_j = 3'd3;
+      end else begin
+        break;
+      end
+      if (rand_a[3]) begin
+        conditional_j = 3'd4;
+      end
+    end while (0);
+  end
+  `signal(CONDITONAL_J, conditional_j);
+
+  logic [2:0] conditional_k;
+  always_comb begin
+    if (rand_b[0]) begin
+      do begin
+        conditional_k = 3'd0;
+        if (rand_a[0]) break;
+        conditional_k = 3'd1;
+        if (rand_a[1]) break;
+        conditional_k = 3'd2;
+        if (rand_a[2]) break;
+        conditional_k = 3'd3;
+        if (rand_a[3]) break;
+        conditional_k = 3'd4;
+      end while (0);
+    end else begin
+      do begin
+        conditional_k = 3'd0;
+        if (rand_a[0]) begin
+          conditional_k = 3'd1;
+        end else begin
+          break;
+        end
+        if (rand_a[1]) begin
+          conditional_k = 3'd2;
+        end else begin
+          break;
+        end
+        if (rand_a[2]) begin
+          conditional_k = 3'd3;
+        end else begin
+          break;
+        end
+        if (rand_a[3]) begin
+          conditional_k = 3'd4;
+        end
+      end while (0);
+    end
+  end
+  `signal(CONDITONAL_K, conditional_k);
+
+  logic [1:0] conditional_l_a;
+  logic [1:0] conditional_l_b;
+  always_comb begin
+    do begin
+      conditional_l_a = 2'd0;
+      if (rand_a[1:0] == 2'd0) break;
+      conditional_l_a = 2'd1;
+      if (rand_a[1:0] == 2'd1) break;
+      conditional_l_a = 2'd2;
+      if (rand_a[1:0] == 2'd2) break;
+      conditional_l_a = 2'd3;
+    end while (0);
+    do begin
+      conditional_l_b = 2'd0;
+      if (rand_b[1:0] == 2'd0) break;
+      conditional_l_b = 2'd1;
+      if (rand_b[1:0] == 2'd1) break;
+      conditional_l_b = 2'd2;
+      if (rand_b[1:0] == 2'd2) break;
+      conditional_l_b = 2'd3;
+    end while (0);
+  end
+  `signal(CONDITONAL_L, {conditional_l_b, conditional_l_a});
+
   logic [7:0] partial_conditional_a;
   always_comb begin
     partial_conditional_a[1:0] = 2'd0;
@@ -225,4 +395,111 @@ module t (
     partial_temporary_a = partial_temporary_tmp;
   end
   `signal(PARTIAL_TEMPORARY, partial_temporary_a);
+
+  logic circular_0_x;
+  logic circular_0_y;
+  logic circular_0_z;
+  always_comb begin
+    circular_0_x = 1'b0;
+    circular_0_y = 1'd0;
+    if (rand_b[0]) begin
+      circular_0_x = 1'b1;
+      if (circular_0_z) begin
+        circular_0_y = 1'd1;
+      end
+    end
+  end
+  assign circular_0_z = circular_0_x & rand_a[0];
+  `signal(CIRCULAR_0, {circular_0_x, circular_0_y, circular_0_z});
+
+  logic [2:0] nsp_a; // Non series-parallel CFG
+  always_comb begin
+    do begin
+      nsp_a = 3'd0; // BB 0 -> BB 1 / BB 2
+      if (rand_a[1:0] == 2'd0) begin
+        nsp_a = 3'd1; // BB 1 -> BB 4
+      end else begin
+        nsp_a = 3'd2; // BB 2 -> BB 3 / BB 4
+        if (rand_a[1:0] == 2'd1) begin
+          nsp_a = 3'd3; // BB3 -> BB 5
+          break;
+        end
+      end
+      nsp_a = 3'd4; // BB 4 -> BB 5
+    end while (0);
+    //nsp_a = 3'd5; // BB 5
+  end
+  `signal(NSP_A, nsp_a);
+
+  logic [2:0] nsp_b; // Non series-parallel CFG
+  always_comb begin
+    do begin
+      nsp_b = 3'd0;
+      if (rand_a[1:0] == 2'd0) begin
+        nsp_b = 3'd1;
+      end else begin
+        nsp_b = 3'd2;
+        if (rand_a[1:0] == 2'd1) begin
+          nsp_b = 3'd3;
+          break;
+        end else begin
+          nsp_b = 3'd4;
+          if (rand_a[1:0] == 2'd2) begin
+            nsp_b = 3'd5;
+          end else begin
+            nsp_b = 3'd6;
+            break;
+          end
+        end
+      end
+      nsp_b = 3'd7;
+    end while (0);
+  end
+  `signal(NSP_B, nsp_b);
+
+  logic [2:0] part_sp_a; // Contains series-parallel sub-graph CFG
+  always_comb begin
+    do begin
+      part_sp_a = 3'd0;
+      if (rand_a[0]) begin
+        part_sp_a = 3'd1;
+        if (rand_a[1]) begin
+          part_sp_a = 3'd2;
+        end
+      end else begin
+        part_sp_a = 3'd3;
+        if (rand_a[2]) begin
+          part_sp_a = 3'd4;
+          if (rand_a[3]) begin
+            part_sp_a = 3'd5;
+          end
+          break;
+        end
+      end
+      part_sp_a = 3'd6;
+      if (rand_a[4]) begin
+        part_sp_a = 3'd7;
+      end
+    end while (0);
+  end
+  `signal(PART_SP_A, part_sp_a);
+
+  logic [1:0] both_break;
+  always_comb begin
+    do begin
+      if (rand_a[0]) begin
+        both_break = 2'd0;
+        break;
+      end else begin
+        both_break = 2'd1;
+        break;
+      end
+      // Unreachable
+      if (rand_a[1]) begin
+        both_break = 2'd2;
+      end
+    end while(0);
+  end
+  `signal(BOTH_BREAK, both_break);
+
 endmodule