Improve automatic selection of logic for Dfg synthesis (#6370)

Reduce set of synthesized logic to be more in-line with what Dfg used to
handle before + drivers of circular variables. This was always the
intention but the previous algorithm was both a bit too eager, and also
missed some circular variables. We can add back more heuristics based on
performance measurements for non-circular logic later.

src/V3Dfg.cpp

@@ -405,11 +405,17 @@ static void dumpDotVertex(std::ostream& os, const DfgVertex& vtx) {
         str = VString::quoteBackslash(str);
         str = VString::quoteAny(str, '"', '\\');
         str = VString::replaceSubstr(str, "\n", "\\l");
+        const char* const colorp = !logicp->selectedForSynthesis() ? "#d0d0ff"  // Pale Blue
+                                   : logicp->nonSynthesizable()    ? "#ffd0d0"  // Pale Red
+                                   : logicp->reverted()            ? "#faffd0"  // Pale Yellow
+                                                                   : "#d0ffd0";  // Pale Green
+
         os << " [label=\"";
         os << str;
         os << "\\n" << cvtToHex(&vtx);
         os << "\"\n";
-        os << ", shape=box, style=\"rounded,filled\", fillcolor=cornsilk, nojustify=true";
+        os << ", shape=box, style=\"rounded,filled\", nojustify=true";
+        os << ", fillcolor=\"" << colorp << "\"";
         os << "]\n";
         return;
     }

src/V3Dfg.h

@@ -756,7 +756,7 @@ class DfgWorklist final {
 
 public:
     // CONSTRUCTOR
-    DfgWorklist(DfgGraph& dfg)
+    explicit DfgWorklist(DfgGraph& dfg)
         : m_dfg{dfg} {}
     VL_UNCOPYABLE(DfgWorklist);
     VL_UNMOVABLE(DfgWorklist);

src/V3DfgSynthesize.cpp

@@ -535,6 +535,7 @@ class AstToDfgSynthesize final {
     AstToDfgConverter<T_Scoped> m_converter;  // The convert instance to use for each construct
     size_t m_nBranchCond = 0;  // Sequence numbers for temporaries
     size_t m_nPathPred = 0;  // Sequence numbers for temporaries
+    DfgWorklist m_toRevert{m_dfg};  // We need a worklist for reverting synthesis
 
     // STATE - for current DfgLogic being synthesized
     DfgLogic* m_logicp = nullptr;  // Current logic vertex we are synthesizing
@@ -1587,46 +1588,102 @@ class AstToDfgSynthesize final {
         return true;
     }
 
-    // Revert synthesis of the given DfgLogic
+    // Revert all DfgLogic in m_toRevert, or DfgLogic driving the DfgUnresolved
-    void revert(DfgLogic& vtx) {
+    // vertices in m_toRevert, and transitively the same for any DfgUnresolved
-        for (DfgVertex* const p : vtx.synth()) VL_DO_DANGLING(p->unlinkDelete(m_dfg), p);
+    // driven by the reverted DfgLogic. Delete all DfgUnresolved involed.
-        vtx.synth().clear();
+    void revert(VDouble0& statCountr) {
-    }
+        m_toRevert.foreach([&](DfgVertex& vtx) {
-
+            // Process DfgLogic
-    // Revert all logic driving the given unresolved driver, delete it,
+            if (DfgLogic* const vtxp = vtx.cast<DfgLogic>()) {
-    // and transitively the same for variables driven by the reverted logic.
+                UASSERT_OBJ(vtxp->selectedForSynthesis(), vtxp, "Shouldn't reach here unselected");
-    void revertTransivelyAndRemove(DfgUnresolved* vtxp, VDouble0& statCountr) {
+                // Revert this logic
-        // The result variable will be driven from Ast code, mark as such
+                UASSERT_OBJ(!vtxp->reverted(), vtxp, "Should be reverting now");
-        vtxp->singleSink()->as<DfgVertexVar>()->setHasModWrRefs();
+                ++statCountr;
-
+                for (DfgVertex* const p : vtxp->synth()) VL_DO_DANGLING(p->unlinkDelete(m_dfg), p);
-        // Gather all logic driving this unresolved driver
+                vtxp->synth().clear();
-        std::vector<DfgLogic*> logicps;
+                vtxp->setReverted();
-        logicps.reserve(vtxp->nInputs());
+                // Add all DfgUnresolved it drives to the work list
-        vtxp->foreachSource([&](DfgVertex& src) {
+                vtxp->foreachSink([&](DfgVertex& snk) {
-            if (DfgLogic* const p = src.cast<DfgLogic>()) logicps.emplace_back(p);
+                    m_toRevert.push_front(*snk.as<DfgUnresolved>());
                     return false;
                 });
+                return;
+            }
 
-        // Delete the unresolved driver
+            // Process DfgUnresolved
+            if (DfgUnresolved* const vtxp = vtx.cast<DfgUnresolved>()) {
+                // The result variable will be driven from Ast code, mark as such
+                vtxp->singleSink()->as<DfgVertexVar>()->setHasModWrRefs();
+                // Add all driving DfgLogic to the work list
+                vtxp->foreachSource([&](DfgVertex& src) {
+                    DfgLogic* const lp = src.cast<DfgLogic>();
+                    if (lp && !lp->reverted()) m_toRevert.push_front(*lp);
+                    return false;
+                });
+                // Delete the DfgUnresolved driver
                 VL_DO_DANGLING(vtxp->unlinkDelete(m_dfg), vtxp);
+                return;
+            }
 
-        // Transitively for the rest
+            // There should be nothing else on the worklist
-        for (DfgLogic* const logicp : logicps) {
+            vtx.v3fatalSrc("Unexpected vertex type");
-            if (!logicp->synth().empty()) {
+        });
-                ++statCountr;
-                revert(*logicp);
-            }
-            while (DfgVertex* const sinkp = logicp->firtsSinkp()) {
-                revertTransivelyAndRemove(sinkp->as<DfgUnresolved>(), statCountr);
-            }
-        }
     }
 
     // Synthesize all of the given vertices
-    void main(const std::vector<DfgLogic*>& logicps) {
+    void main() {
         //-------------------------------------------------------------------
-        UINFO(5, "Step 1: Attempting to synthesize each of the given DfgLogic");
+        UINFO(5, "Step 0: Remove all DfgLogic not selected for synthesis");
-        for (DfgLogic* const logicp : logicps) {
+        for (DfgVertex* const vtxp : m_dfg.opVertices().unlinkable()) {
+            // Only processing DfgUnresolved
+            if (!vtxp->is<DfgUnresolved>()) continue;
+            bool anySelected = false;
+            bool anyUnselected = false;
+            vtxp->foreachSource([&](DfgVertex& src) {
+                const DfgLogic& logic = *src.as<DfgLogic>();
+                if (logic.selectedForSynthesis()) {
+                    anySelected = true;
+                } else {
+                    anyUnselected = true;
+                }
+                return false;
+            });
+            // There should be a driver
+            UASSERT_OBJ(anySelected || anyUnselected, vtxp, "'DfgUnresolved' with no driver");
+            // All drivers should be selected or all should be unselected
+            UASSERT_OBJ(!(anySelected && anyUnselected), vtxp, "Invalid 'DfgLogic' selection");
+            // If all drivers are unselected, delete this DfgUnresolved here
+            if (anyUnselected) {
+                // The result variable will be driven from Ast code, mark as such
+                vtxp->singleSink()->as<DfgVertexVar>()->setHasModWrRefs();
+                VL_DO_DANGLING(vtxp->unlinkDelete(m_dfg), vtxp);
+            }
+        }
+        for (DfgVertex* const vtxp : m_dfg.opVertices().unlinkable()) {
+            // Only processing DfgLogic
+            DfgLogic* const logicp = vtxp->cast<DfgLogic>();
+            if (!logicp) continue;
+            if (logicp->selectedForSynthesis()) continue;
+            // There should be no sinks left for unselected DfgLogic, delete them here
+            UASSERT_OBJ(!logicp->hasSinks(), vtxp, "Unselected 'DfgLogic' with sinks remaining");
+            // Input variables will be read in Ast code, mark as such
+            logicp->foreachSource([](DfgVertex& src) {
+                src.as<DfgVertexVar>()->setHasModRdRefs();
+                return false;
+            });
+            VL_DO_DANGLING(logicp->unlinkDelete(m_dfg), logicp);
+        }
+        debugDump("synth-selected");
+
+        //-------------------------------------------------------------------
+        UINFO(5, "Step 1: Attempting to synthesize each of the selected DfgLogic");
+        for (DfgVertex& vtx : m_dfg.opVertices()) {
+            DfgLogic* const logicp = vtx.cast<DfgLogic>();
+            if (!logicp) continue;
+
+            // We should only have DfgLogic remaining that was selected for synthesis
+            UASSERT_OBJ(logicp->selectedForSynthesis(), logicp, "Unselected DfgLogic remains");
+
             // Debug aid
             if (VL_UNLIKELY(s_dfgSynthDebugLimit)) {
                 if (s_dfgSynthDebugCount == s_dfgSynthDebugLimit) break;
@@ -1641,27 +1698,18 @@ class AstToDfgSynthesize final {
                 }
             }
 
-            // Synthesize it, revert partial construction if failed
+            // Synthesize it, if failed, enqueue for reversion
-            if (!synthesize(*logicp)) revert(*logicp);
+            if (!synthesize(*logicp)) {
+                logicp->setNonSynthesizable();
+                m_toRevert.push_front(*logicp);
+            }
         }
         debugDump("synth-converted");
 
         //-------------------------------------------------------------------
         UINFO(5, "Step 2: Revert drivers of variables with unsynthesizeable drivers");
-        // We do this as the variables might be multi-driven, we just can't know at this point
+        // We do this as the variables might be multi-driven, we can't know if synthesis failed
-        for (const DfgVertexVar& var : m_dfg.varVertices()) {
+        revert(m_ctx.m_synt.revertNonSyn);
-            if (!var.srcp()) continue;
-            DfgUnresolved* const unresolvedp = var.srcp()->cast<DfgUnresolved>();
-            if (!unresolvedp) break;  // Stop when reached the synthesized temporaries
-
-            // Check if any driver have failed to synthesize
-            const bool failed = unresolvedp->foreachSource([&](DfgVertex& src) {
-                DfgLogic* const driverp = src.cast<DfgLogic>();
-                return driverp && driverp->synth().empty();
-            });
-            // Revert all logic involved
-            if (failed) revertTransivelyAndRemove(unresolvedp, m_ctx.m_synt.revertNonSyn);
-        }
         debugDump("synth-reverted");
 
         //-------------------------------------------------------------------
@@ -1689,6 +1737,7 @@ class AstToDfgSynthesize final {
                 if (normalizeDrivers(var, drivers)) return makeSplice(var, drivers);
                 // If mutlidriven, record and ignore
                 multidrivenps.emplace_back(&var);
+                m_toRevert.push_front(*unresolvedp);
                 return nullptr;
             }();
             // Bail if multidriven
@@ -1697,16 +1746,10 @@ class AstToDfgSynthesize final {
             const bool newEntry = resolvedDrivers.emplace(&var, resolvedp).second;
             UASSERT_OBJ(newEntry, &var, "Dupliacte driver");
         }
+        // Mark as multidriven for future DFG runs - here, so we get all warnings above
+        for (const DfgVertexVar* const vtxp : multidrivenps) vtxp->varp()->setDfgMultidriven();
         // Revert and remove drivers of multi-driven variables
-        for (const DfgVertexVar* const vtxp : multidrivenps) {
+        revert(m_ctx.m_synt.revertMultidrive);
-            // Mark as multidriven for future DFG runs - here, so we get all warning before
-            vtxp->varp()->setDfgMultidriven();
-            // Might not have a driver if transitively removed on an earlier iteration
-            if (!vtxp->srcp()) continue;
-            // Revert all logic involved
-            DfgUnresolved* const unresolvedp = vtxp->srcp()->as<DfgUnresolved>();
-            revertTransivelyAndRemove(unresolvedp, m_ctx.m_synt.revertMultidrive);
-        }
         // Replace all DfgUnresolved with the resolved drivers
         for (const DfgVertexVar& var : m_dfg.varVertices()) {
             if (!var.srcp()) continue;
@@ -1720,7 +1763,7 @@ class AstToDfgSynthesize final {
         debugDump("synth-resolved");
 
         //-------------------------------------------------------------------
-        UINFO(5, "Step 4: Remove all DfgLogic and DfgUnresolved");
+        UINFO(5, "Step 4: Remove all remaining DfgLogic and DfgUnresolved");
         for (DfgVertex* const vtxp : m_dfg.opVertices().unlinkable()) {
             // Previous step should have removed all DfgUnresolved
             UASSERT_OBJ(!vtxp->is<DfgUnresolved>(), vtxp, "DfgUnresolved remains");
@@ -1776,17 +1819,30 @@ class AstToDfgSynthesize final {
             }
         }
         debugDump("synth-rmsplice");
+    }
 
-        //-------------------------------------------------------------------
+    // CONSTRUCTOR
+    AstToDfgSynthesize(DfgGraph& dfg, V3DfgSynthesisContext& ctx)
+        : m_dfg{dfg}
+        , m_ctx{ctx}
+        , m_converter{dfg, ctx} {
+        main();
+    }
+
+public:
+    static void apply(DfgGraph& dfg, V3DfgSynthesisContext& ctx) {
+        AstToDfgSynthesize{dfg, ctx};
+
+        // Final step outside, as both AstToDfgSynthesize and removeUnused used DfgUserMap
         UINFO(5, "Step 6: Remove all unused vertices");
-        V3DfgPasses::removeUnused(m_dfg);
+        V3DfgPasses::removeUnused(dfg);
-        debugDump("synth-rmunused");
+        if (dumpDfgLevel() >= 9) dfg.dumpDotFilePrefixed(ctx.prefix() + "synth-rmunused");
 
         // No operation vertex should have multiple sinks. Cyclic decomoposition
         // depends on this and it can easily be ensured by using temporaries.
         // Also, all sources should be connected at this point
         if (v3Global.opt.debugCheck()) {
-            for (DfgVertex& vtx : m_dfg.opVertices()) {
+            for (DfgVertex& vtx : dfg.opVertices()) {
                 UASSERT_OBJ(!vtx.hasMultipleSinks(), &vtx, "Operation has multiple sinks");
                 for (size_t i = 0; i < vtx.nInputs(); ++i) {
                     UASSERT_OBJ(vtx.inputp(i), &vtx, "Unconnected source operand");
@@ -1794,79 +1850,83 @@ class AstToDfgSynthesize final {
             }
         }
     }
-
-    // CONSTRUCTOR
-    AstToDfgSynthesize(DfgGraph& dfg, const std::vector<DfgLogic*>& logicps,
-                       V3DfgSynthesisContext& ctx)
-        : m_dfg{dfg}
-        , m_ctx{ctx}
-        , m_converter{dfg, ctx} {}
-
-public:
-    static void apply(DfgGraph& dfg, const std::vector<DfgLogic*>& logicps,
-                      V3DfgSynthesisContext& ctx) {
-        AstToDfgSynthesize{dfg, logicps, ctx}.main(logicps);
-    }
 };
 
-void V3DfgPasses::synthesize(DfgGraph& dfg, V3DfgContext& ctx) {
+// Decide which DfgLogic to attempt to synthesize
-    // The vertices to synthesize
+static void dfgSelectLogicForSynthesis(DfgGraph& dfg) {
-    std::vector<DfgLogic*> logicps;
-
-    if (v3Global.opt.fDfgSynthesizeAll()) {
     // If we are told to synthesize everything, we will do so ...
+    if (v3Global.opt.fDfgSynthesizeAll()) {
         for (DfgVertex& vtx : dfg.opVertices()) {
-            if (DfgLogic* const logicp = vtx.cast<DfgLogic>()) logicps.emplace_back(logicp);
+            if (DfgLogic* const logicp = vtx.cast<DfgLogic>()) logicp->setSelectedForSynthesis();
+        }
+        return;
     }
-    } else {
-        // Otherwise figure out which vertices are worth synthesizing.
 
-        // Find cycles
+    // Otherwise figure out which vertices are likely worth synthesizing.
+
+    // Bather circular variables
+    std::vector<DfgVertexVar*> circularVarps;
+    {
         DfgUserMap<uint64_t> scc = dfg.makeUserMap<uint64_t>();
         V3DfgPasses::colorStronglyConnectedComponents(dfg, scc);
-
-        // First, gather variables, we will then attempt to synthesize all their drivers
-        std::vector<DfgVertexVar*> varps;
         for (DfgVertexVar& var : dfg.varVertices()) {
-            // Can ignore variables with no drivers
+            if (!scc.at(var)) continue;
-            if (!var.srcp()) continue;
+            // This is a circular variable
-
+            circularVarps.emplace_back(&var);
-            // Circular variable - synthesize
+        }
-            if (scc.at(var)) {
-                varps.emplace_back(&var);
-                continue;
     }
 
-            // Must be driven from a DfgUnresolved at this point, pick it up
+    // We need to expand the selection to cover all drivers, use a work list
-            const DfgUnresolved* const unresolvedp = var.srcp()->as<DfgUnresolved>();
+    DfgWorklist worklist{dfg};
 
-            // Inspect drivers to figure out if we should synthesize them
+    // Synthesize all drivers of circular variables
-            const bool doIt = unresolvedp->foreachSource([&](const DfgVertex& src) {
+    for (const DfgVertexVar* const varp : circularVarps) {
-                const DfgLogic* const logicp = src.as<DfgLogic>();
+        varp->srcp()->as<DfgUnresolved>()->foreachSource([&](DfgVertex& driver) {
-                // Synthesize continuous assignments (this is the earlier behaviour).
+            worklist.push_front(*driver.as<DfgLogic>());
-                // 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 VN_IS(logicp->nodep(), AssignW)  //
-                       || (logicp->cfg().nBlocks() <= 4 && logicp->cfg().nEdges() <= 4);
-            });
-            if (doIt) varps.emplace_back(&var);
-        }
-
-        // Gather all drivers of the selected variables
-        const VNUser2InUse user2InUse;  // AstNode (logic) -> bool: already collected
-        for (const DfgVertexVar* const varp : varps) {
-            varp->srcp()->as<DfgUnresolved>()->foreachSource([&](DfgVertex& source) {
-                DfgLogic* const logicp = source.as<DfgLogic>();
-                if (!logicp->nodep()->user2Inc()) logicps.emplace_back(logicp);
             return false;
         });
     }
+
+    // Synthesize all AssignW and simple blocks driving exactly one variable
+    // This is approximately the old default behaviour of Dfg
+    for (DfgVertex& vtx : dfg.opVertices()) {
+        DfgLogic* const logicp = vtx.cast<DfgLogic>();
+        if (!logicp) continue;
+        if (VN_IS(logicp->nodep(), AssignW)) {
+            worklist.push_front(*logicp);
+            continue;
+        }
+        const CfgGraph& cfg = logicp->cfg();
+        if (!logicp->hasMultipleSinks() && cfg.nBlocks() <= 4 && cfg.nEdges() <= 4) {
+            worklist.push_front(*logicp);
+        }
     }
 
-    // Synthesize them - also removes un-synthesized DfgLogic, so must run even if logicps.empty()
+    // Now expand to cover all logic driving the same set of variables and mark
+    worklist.foreach([&](DfgVertex& vtx) {
+        DfgLogic& logic = *vtx.as<DfgLogic>();
+        UASSERT_OBJ(!logic.selectedForSynthesis(), &vtx, "Should not be visited twice");
+        // Mark as selected for synthesis
+        logic.setSelectedForSynthesis();
+        // Enqueue all other logic driving the same variables as this one
+        logic.foreachSink([&](DfgVertex& sink) {
+            sink.as<DfgUnresolved>()->foreachSource([&](DfgVertex& sibling) {
+                DfgLogic& siblingLogic = *sibling.as<DfgLogic>();
+                if (!siblingLogic.selectedForSynthesis()) worklist.push_front(siblingLogic);
+                return false;
+            });
+            return false;
+        });
+    });
+}
+
+void V3DfgPasses::synthesize(DfgGraph& dfg, V3DfgContext& ctx) {
+    // Select which DfgLogic to attempt to synthesize
+    dfgSelectLogicForSynthesis(dfg);
+    // Synthesize them - also removes un-synthesized DfgLogic, so must run even if nothing selected
     if (dfg.modulep()) {
-        AstToDfgSynthesize</* T_Scoped: */ false>::apply(dfg, logicps, ctx.m_synthContext);
+        AstToDfgSynthesize</* T_Scoped: */ false>::apply(dfg, ctx.m_synthContext);
     } else {
-        AstToDfgSynthesize</* T_Scoped: */ true>::apply(dfg, logicps, ctx.m_synthContext);
+        AstToDfgSynthesize</* T_Scoped: */ true>::apply(dfg, ctx.m_synthContext);
     }
 }

src/V3DfgVertices.h

@@ -468,6 +468,9 @@ class DfgLogic final : public DfgVertexVariadic {
     const std::unique_ptr<CfgGraph> m_cfgp;
     // Vertices this logic was synthesized into. Excluding variables
     std::vector<DfgVertex*> m_synth;
+    bool m_selectedForSynthesis = false;  // Logic selected for synthesis
+    bool m_nonSynthesizable = false;  // Logic is not synthesizeable (by DfgSynthesis)
+    bool m_reverted = false;  // Logic was synthesized (in part if non synthesizable) then reverted
 
 public:
     DfgLogic(DfgGraph& dfg, AstAssignW* nodep, AstScope* scopep)
@@ -496,6 +499,12 @@ public:
     const CfgGraph& cfg() const { return *m_cfgp; }
     std::vector<DfgVertex*>& synth() { return m_synth; }
     const std::vector<DfgVertex*>& synth() const { return m_synth; }
+    bool selectedForSynthesis() const { return m_selectedForSynthesis; }
+    void setSelectedForSynthesis() { m_selectedForSynthesis = true; }
+    bool nonSynthesizable() const { return m_nonSynthesizable; }
+    void setNonSynthesizable() { m_nonSynthesizable = true; }
+    bool reverted() const { return m_reverted; }
+    void setReverted() { m_reverted = true; }
 };
 
 class DfgUnresolved final : public DfgVertexVariadic {

test_regress/t/t_func_crc.py

@@ -18,6 +18,6 @@ test.compile(
 test.execute()
 
 if test.vlt:
-    test.file_grep(test.stats, r'Optimizations, Const bit op reduction\s+(\d+)', 3434)
+    test.file_grep(test.stats, r'Optimizations, Const bit op reduction\s+(\d+)', 3888)
 
 test.passes()

test_regress/t/t_hier_block_chained.py

@@ -32,9 +32,9 @@ test.compile(
 
 if test.vltmt:
     test.file_grep(test.obj_dir + "/V" + test.name + "__hier.dir/V" + test.name + "__stats.txt",
-                   r'Optimizations, Thread schedule count\s+(\d+)', 3)
+                   r'Optimizations, Thread schedule count\s+(\d+)', 2)
     test.file_grep(test.obj_dir + "/V" + test.name + "__hier.dir/V" + test.name + "__stats.txt",
-                   r'Optimizations, Thread schedule total tasks\s+(\d+)', 4)
+                   r'Optimizations, Thread schedule total tasks\s+(\d+)', 3)
 
 test.execute()