Internals: Remove DfgGraph::clone

Only used by V3DfgBreakCycles, which today either improves the graph, or
leaves it unchanged. Remove unnecessary complexity.

src/V3Dfg.cpp

@@ -34,180 +34,6 @@ DfgGraph::~DfgGraph() {
     forEachVertex([&](DfgVertex& vtx) { vtx.unlinkDelete(*this); });
 }
 
-std::unique_ptr<DfgGraph> DfgGraph::clone() const {
-    // Create the new graph
-    DfgGraph* const clonep = new DfgGraph{name()};
-
-    // Map from original vertex to clone
-    std::unordered_map<const DfgVertex*, DfgVertex*> vtxp2clonep(size() * 2);
-
-    // Clone constVertices
-    for (const DfgConst& vtx : m_constVertices) {
-        DfgConst* const cp = new DfgConst{*clonep, vtx.fileline(), vtx.num()};
-        vtxp2clonep.emplace(&vtx, cp);
-    }
-    // Clone variable vertices
-    for (const DfgVertexVar& vtx : m_varVertices) {
-        const DfgVertexVar* const vp = vtx.as<DfgVertexVar>();
-        DfgVertexVar* cp = nullptr;
-
-        switch (vtx.type()) {
-        case VDfgType::VarArray: {
-            cp = new DfgVarArray{*clonep, vp->vscp()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::VarPacked: {
-            cp = new DfgVarPacked{*clonep, vp->vscp()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        default: {
-            vtx.v3fatalSrc("Unhandled variable vertex type: " + vtx.typeName());
-            VL_UNREACHABLE;
-            break;
-        }
-        }
-
-        if (AstVarScope* const tmpForp = vp->tmpForp()) cp->tmpForp(tmpForp);
-    }
-    // Clone ast reference vertices
-    for (const DfgVertexAst& vtx : m_astVertices) {  // LCOV_EXCL_START
-        switch (vtx.type()) {
-        case VDfgType::AstRd: {
-            const DfgAstRd* const vp = vtx.as<DfgAstRd>();
-            DfgAstRd* const cp = new DfgAstRd{*clonep, vp->exprp(), vp->inSenItem(), vp->inLoop()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        default: {
-            vtx.v3fatalSrc("Unhandled ast reference vertex type: " + vtx.typeName());
-            VL_UNREACHABLE;
-            break;
-        }
-        }
-    }  // LCOV_EXCL_STOP
-    // Clone operation vertices
-    for (const DfgVertex& vtx : m_opVertices) {
-        switch (vtx.type()) {
-#include "V3Dfg__gen_clone_cases.h"  // From ./astgen
-        case VDfgType::CReset: {  // LCOV_EXCL_START - No algorithm actually hits this today
-            DfgCReset* const cp = new DfgCReset{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }  // LCOV_EXCL_STOP
-        case VDfgType::MatchMasked: {
-            DfgMatchMasked* const cp = new DfgMatchMasked{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::Sel: {
-            DfgSel* const cp = new DfgSel{*clonep, vtx.fileline(), vtx.dtype()};
-            cp->lsb(vtx.as<DfgSel>()->lsb());
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::Rep: {
-            DfgRep* const cp = new DfgRep{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::UnitArray: {
-            DfgUnitArray* const cp = new DfgUnitArray{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::Mux: {
-            DfgMux* const cp = new DfgMux{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::SpliceArray: {
-            DfgSpliceArray* const cp = new DfgSpliceArray{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::SplicePacked: {
-            DfgSplicePacked* const cp = new DfgSplicePacked{*clonep, vtx.fileline(), vtx.dtype()};
-            vtxp2clonep.emplace(&vtx, cp);
-            break;
-        }
-        case VDfgType::Logic: {
-            vtx.v3fatalSrc("DfgLogic cannot be cloned");
-            VL_UNREACHABLE;
-            break;
-        }
-        case VDfgType::Unresolved: {
-            vtx.v3fatalSrc("DfgUnresolved cannot be cloned");
-            VL_UNREACHABLE;
-            break;
-        }
-        case VDfgType::AstRd:  // LCOV_EXCL_START
-        case VDfgType::Const:
-        case VDfgType::VarArray:
-        case VDfgType::VarPacked: {
-            vtx.v3fatalSrc("Vertex should have been handled above: " + vtx.typeName());
-            VL_UNREACHABLE;
-            break;
-        }  // LCOV_EXCL_STOP
-        }
-    }
-    UASSERT(size() == clonep->size(), "Size of clone should be the same");
-
-    // Constants have no inputs
-    // Hook up inputs of cloned variables
-    for (const DfgVertexVar& vtx : m_varVertices) {
-        DfgVertexVar* const cp = vtxp2clonep.at(&vtx)->as<DfgVertexVar>();
-        if (const DfgVertex* const srcp = vtx.srcp()) cp->srcp(vtxp2clonep.at(srcp));
-        if (const DfgVertex* const defp = vtx.defaultp()) cp->defaultp(vtxp2clonep.at(defp));
-    }
-    // Hook up inputs of cloned ast references
-    for (const DfgVertexAst& vtx : m_astVertices) {  // LCOV_EXCL_START
-        switch (vtx.type()) {
-        case VDfgType::AstRd: {
-            const DfgAstRd* const vp = vtx.as<DfgAstRd>();
-            DfgAstRd* const cp = vtxp2clonep.at(&vtx)->as<DfgAstRd>();
-            if (const DfgVertex* const srcp = vp->srcp()) cp->srcp(vtxp2clonep.at(srcp));
-            break;
-        }
-        default: {
-            vtx.v3fatalSrc("Unhandled DfgVertexAst sub type: " + vtx.typeName());
-            VL_UNREACHABLE;
-            break;
-        }
-        }
-    }  // LCOV_EXCL_STOP
-    // Hook up inputs of cloned operation vertices
-    for (const DfgVertex& vtx : m_opVertices) {
-        if (vtx.is<DfgVertexVariadic>()) {
-            switch (vtx.type()) {
-            case VDfgType::SpliceArray:
-            case VDfgType::SplicePacked: {
-                const DfgVertexSplice* const vp = vtx.as<DfgVertexSplice>();
-                DfgVertexSplice* const cp = vtxp2clonep.at(vp)->as<DfgVertexSplice>();
-                vp->foreachDriver([&](const DfgVertex& src, uint32_t lo, FileLine* flp) {
-                    cp->addDriver(vtxp2clonep.at(&src), lo, flp);
-                    return false;
-                });
-                break;
-            }
-            default: {
-                vtx.v3fatalSrc("Unhandled DfgVertexVariadic sub type: " + vtx.typeName());
-                VL_UNREACHABLE;
-                break;
-            }
-            }
-        } else {
-            DfgVertex* const cp = vtxp2clonep.at(&vtx);
-            for (size_t i = 0; i < vtx.nInputs(); ++i) {
-                cp->inputp(i, vtxp2clonep.at(vtx.inputp(i)));
-            }
-        }
-    }
-
-    return std::unique_ptr<DfgGraph>{clonep};
-}
-
 void DfgGraph::mergeGraphs(std::vector<std::unique_ptr<DfgGraph>>&& otherps) {
     if (otherps.empty()) return;
 

src/V3Dfg.h

@@ -487,9 +487,6 @@ public:
         for (const DfgVertex& vtx : m_opVertices) f(vtx);
     }
 
-    // Return an identical, independent copy of this graph. Vertex and edge order might differ.
-    std::unique_ptr<DfgGraph> clone() const VL_MT_DISABLED;
-
     // Merge contents of other graphs into this graph. Deletes the other graphs.
     // DfgVertexVar instances representing the same Ast variable are unified.
     void mergeGraphs(std::vector<std::unique_ptr<DfgGraph>>&& otherps) VL_MT_DISABLED;

src/V3DfgBreakCycles.cpp

@@ -1583,35 +1583,18 @@ public:
     }
 };
 
-std::pair<std::unique_ptr<DfgGraph>, bool>  //
+bool breakCycles(DfgGraph& dfg, V3DfgBreakCyclesContext& ctx) {
-breakCycles(const DfgGraph& dfg, V3DfgContext& ctx) {
     // Shorthand for dumping graph at given dump level
     const auto dump = [&](int level, const DfgGraph& dfg, const std::string& name) {
         if (dumpDfgLevel() >= level) dfg.dumpDotFilePrefixed("breakCycles-" + name);
     };
 
-    // Can't do much with trivial things ('a = a' or 'a[1] = a[0]'), so bail
-    if (dfg.size() <= 2) {
-        UINFO(7, "Graph is trivial");
-        dump(9, dfg, "trivial");
-        ++ctx.m_breakCyclesContext.m_nTrivial;
-        return {nullptr, false};
-    }
-
     // AstNetlist/AstNodeModule user2 used as sequence numbers for temporaries
     const VNUser2InUse user2InUse;
 
     // Show input for debugging
     dump(7, dfg, "input");
 
-    // We might fail to make any improvements, so first create a clone of the
-    // graph. This is what we will be working on, and return if successful.
-    // Do not touch the input graph.
-    std::unique_ptr<DfgGraph> resultp = dfg.clone();
-    // Just shorthand for code below
-    DfgGraph& res = *resultp;
-    dump(9, res, "clone");
-
     // How many improvements have we made
     size_t nImprovements = 0;
     size_t prevNImprovements;
@@ -1619,16 +1602,16 @@ breakCycles(const DfgGraph& dfg, V3DfgContext& ctx) {
     // Iterate while an improvement can be made and the graph is still cyclic
     do {
         // Compute SCCs
-        SccInfo sccInfo{res};
+        SccInfo sccInfo{dfg};
 
         // Fix up independent ranges in vertices
         UINFO(9, "New iteration after " << nImprovements << " improvements");
         prevNImprovements = nImprovements;
-        const size_t nFixed = FixUp::apply(res, sccInfo);
+        const size_t nFixed = FixUp::apply(dfg, sccInfo);
         if (nFixed) {
             nImprovements += nFixed;
-            ctx.m_breakCyclesContext.m_nImprovements += nFixed;
+            ctx.m_nImprovements += nFixed;
-            dump(9, res, "FixUp");
+            dump(9, dfg, "FixUp");
         }
 
         // Validate SccInfo if in debug mode
@@ -1637,42 +1620,38 @@ breakCycles(const DfgGraph& dfg, V3DfgContext& ctx) {
         // Congrats if it has become acyclic
         if (!sccInfo.isCyclic()) {
             UINFO(7, "Graph became acyclic after " << nImprovements << " improvements");
-            dump(7, res, "result-acyclic");
+            dump(7, dfg, "result-acyclic");
-            ++ctx.m_breakCyclesContext.m_nFixed;
+            ++ctx.m_nFixed;
-            return {std::move(resultp), true};
+            return true;
         }
     } while (nImprovements != prevNImprovements);
 
+    // Debug dump
     if (dumpDfgLevel() >= 9) {
-        const SccInfo sccInfo{res};
+        const SccInfo sccInfo{dfg};
-        res.dumpDotFilePrefixed("breakCycles-remaining", [&](const DfgVertex& vtx) {
+        dfg.dumpDotFilePrefixed("breakCycles-remaining", [&](const DfgVertex& vtx) {
             return sccInfo.get(vtx);  //
         });
     }
 
-    // If an improvement was made, return the still cyclic improved graph
+    // Accounting
     if (nImprovements) {
         UINFO(7, "Graph was improved " << nImprovements << " times");
-        dump(7, res, "result-improved");
+        dump(7, dfg, "result-improved");
-        ++ctx.m_breakCyclesContext.m_nImproved;
+        ++ctx.m_nImproved;
-        return {std::move(resultp), false};
+    } else {
+        UINFO(7, "Graph NOT improved");
+        dump(7, dfg, "result-original");
+        ++ctx.m_nUnchanged;
     }
-
+    return false;
-    // No improvement was made
-    UINFO(7, "Graph NOT improved");
-    dump(7, res, "result-original");
-    ++ctx.m_breakCyclesContext.m_nUnchanged;
-    return {nullptr, false};
 }
 
 }  //namespace V3DfgBreakCycles
 
-std::pair<std::unique_ptr<DfgGraph>, bool>  //
+bool V3DfgPasses::breakCycles(DfgGraph& dfg, V3DfgContext& ctx) {
-V3DfgPasses::breakCycles(const DfgGraph& dfg, V3DfgContext& ctx) {
+    const bool res = V3DfgBreakCycles::breakCycles(dfg, ctx.m_breakCyclesContext);
-    auto pair = V3DfgBreakCycles::breakCycles(dfg, ctx);
+    if (v3Global.opt.debugCheck()) V3DfgPasses::typeCheck(dfg);
-    if (pair.first) {
+    V3DfgPasses::removeUnused(dfg);
-        if (v3Global.opt.debugCheck()) V3DfgPasses::typeCheck(*pair.first);
+    return res;
-        V3DfgPasses::removeUnused(*pair.first);
-    }
-    return pair;
 }

src/V3DfgContext.h

@@ -100,9 +100,8 @@ class V3DfgBreakCyclesContext final : public V3DfgSubContext {
 public:
     // STATE
     VDouble0 m_nFixed;  // Number of graphs that became acyclic
-    VDouble0 m_nImproved;  // Number of graphs that were imporoved but still cyclic
+    VDouble0 m_nImproved;  // Number of graphs that were improved but still cyclic
     VDouble0 m_nUnchanged;  // Number of graphs that were left unchanged
-    VDouble0 m_nTrivial;  // Number of graphs that were not changed
     VDouble0 m_nImprovements;  // Number of changes made to graphs
 
 private:
@@ -112,7 +111,6 @@ private:
         addStat("made acyclic", m_nFixed);
         addStat("improved", m_nImproved);
         addStat("left unchanged", m_nUnchanged);
-        addStat("trivial", m_nTrivial);
         addStat("changes applied", m_nImprovements);
     }
 };

src/V3DfgOptimizer.cpp

@@ -126,19 +126,15 @@ class DataflowOptimize final {
         std::vector<std::unique_ptr<DfgGraph>> madeAcyclicComponents;
         if (v3Global.opt.fDfgBreakCycles()) {
             for (auto it = cyclicComps.begin(); it != cyclicComps.end();) {
-                auto result = V3DfgPasses::breakCycles(**it, m_ctx);
+                const bool madeAcyclic = V3DfgPasses::breakCycles(**it, m_ctx);
-                if (!result.first) {
+                // If not made acyclic, keep it in 'cyclicComps'
-                    // No improvement, moving on.
+                if (!madeAcyclic) {
                     ++it;
-                } else if (!result.second) {
+                    continue;
-                    // Improved, but still cyclic. Replace the original cyclic component.
-                    *it = std::move(result.first);
-                    ++it;
-                } else {
-                    // Result became acyclic. Move to madeAcyclicComponents, delete original.
-                    madeAcyclicComponents.emplace_back(std::move(result.first));
-                    it = cyclicComps.erase(it);
                 }
+                // Otherwise move to 'madeAcyclicComponents'
+                madeAcyclicComponents.emplace_back(std::move(*it));
+                it = cyclicComps.erase(it);
             }
         }
         // Merge those that were made acyclic back to the graph, this enables optimizing more

src/V3DfgPasses.h

@@ -44,14 +44,9 @@ void synthesize(DfgGraph&, V3DfgContext&) VL_MT_DISABLED;
 // Remove redundant selects
 void removeSelects(DfgGraph& dfg, V3DfgRemoveSelectsContext& ctx) VL_MT_DISABLED;
 // Attempt to make the given cyclic graph into an acyclic, or "less cyclic"
-// equivalent. If the returned pointer is null, then no improvement was
+// equivalent. Genuine combinational cycles can exist, so this might be
-// possible on the input graph. Otherwise the returned graph is an improvement
+// unsuccessful. Returns true if the graph became acyclic, false otherwise.
-// on the input graph, with at least some cycles eliminated. The returned
+bool breakCycles(DfgGraph&, V3DfgContext&) VL_MT_DISABLED;
-// graph is always independent of the original. If an imporoved graph is
-// returned, then the returned 'bool' flag indicated if the returned graph is
-// acyclic (flag 'true'), or still cyclic (flag 'false').
-std::pair<std::unique_ptr<DfgGraph>, bool>  //
-breakCycles(const DfgGraph&, V3DfgContext&) VL_MT_DISABLED;
 // Construct binary to oneHot decoders
 void binToOneHot(DfgGraph&, V3DfgBinToOneHotContext&) VL_MT_DISABLED;
 // Common subexpression elimination

src/astgen

@@ -1279,29 +1279,6 @@ def write_dfg_auto_classes(filename):
         fh.write("\n")
 
 
-def write_dfg_clone_cases(filename):
-    with open_file(filename) as fh:
-
-        def emitBlock(pattern, **fmt):
-            fh.write(textwrap.dedent(pattern).format(**fmt))
-
-        for node in DfgVertexList:
-            # Only generate code for automatically derived leaf nodes
-            if (node.file is not None) or not node.isLeaf:
-                continue
-
-            emitBlock('''\
-                      case VDfgType::{t}: {{
-                        Dfg{t}* const cp = new Dfg{t}{{*clonep, vtx.fileline(), vtx.dtype()}};
-                        vtxp2clonep.emplace(&vtx, cp);
-                        break;
-                      }}
-                      ''',
-                      t=node.name,
-                      s=node.superClass.name)
-        fh.write("\n")
-
-
 def write_dfg_ast_to_dfg(filename):
     with open_file(filename) as fh:
         for node in DfgVertexList:
@@ -1499,7 +1476,6 @@ if Args.classes:
     write_type_tests("Dfg", DfgVertexList)
     write_dfg_macros("V3Dfg__gen_macros.h")
     write_dfg_auto_classes("V3Dfg__gen_auto_classes.h")
-    write_dfg_clone_cases("V3Dfg__gen_clone_cases.h")
     write_dfg_ast_to_dfg("V3Dfg__gen_ast_to_dfg.h")
     write_dfg_dfg_to_ast("V3Dfg__gen_dfg_to_ast.h")