From 2be257369acc0eab28c2f427a66194e8dac0385a Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Thu, 24 Jul 2025 15:31:09 +0100 Subject: [PATCH] Fix component numbers of new Vertices in V3DfgBreakCycles (#6228) Fix component numbers of new Vertices in V3DfgBreakCycles Fixes part of #6225 --- src/V3DfgBreakCycles.cpp | 32 ++++++++++++++++++++++------ test_regress/t/t_dfg_break_cycles.py | 1 + 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/V3DfgBreakCycles.cpp b/src/V3DfgBreakCycles.cpp index 8e7fd7804..46a53c6c2 100644 --- a/src/V3DfgBreakCycles.cpp +++ b/src/V3DfgBreakCycles.cpp @@ -210,7 +210,12 @@ class TraceDriver final : public DfgVisitor { // Create and return a new Vertex and add it to m_newVtxps. Fileline is // taken from 'refp', but 'refp' is otherwise not used. You should // always use this to create new vertices, so unused ones (if a trace - // eventually fails) can be cleaned up at the end. + // eventually fails) can be cleaned up at the end. This also sets the + // vertex user to 0, indicating the new vertex is not part of a + // strongly connected component. This should always be true, as all the + // vertices we create here are driven from outside the component we are + // trying to escape, and will sink into that component. Given those are + // separate SCCs, these new vertices must be acyclic. template Vertex* make(const DfgVertex* refp, uint32_t width) { static_assert(std::is_base_of::value // @@ -229,6 +234,7 @@ class TraceDriver final : public DfgVisitor { if VL_CONSTEXPR_CXX17 (std::is_same::value) { DfgConst* const vtxp = new DfgConst{m_dfg, refp->fileline(), width}; + vtxp->template setUser(0); m_newVtxps.emplace_back(vtxp); return reinterpret_cast(vtxp); } else { @@ -239,6 +245,7 @@ class TraceDriver final : public DfgVisitor { Vertex>::type; AstNodeDType* const dtypep = DfgVertex::dtypeForWidth(width); Vtx* const vtxp = new Vtx{m_dfg, refp->fileline(), dtypep}; + vtxp->template setUser(0); m_newVtxps.emplace_back(vtxp); return reinterpret_cast(vtxp); } @@ -276,7 +283,7 @@ class TraceDriver final : public DfgVisitor { // Trace the vertex onStackr = true; - if (vtxp->user() != m_component) { + if (vtxp->getUser() != m_component) { // If the currently traced vertex is in a different component, // then we found what we were looking for. if (msb != vtxp->width() - 1 || lsb != 0) { @@ -579,7 +586,7 @@ public: // waste a lot of compute. static DfgVertex* apply(DfgGraph& dfg, DfgVertex* vtxp, uint32_t lsb, uint32_t width, bool aggressive) { - TraceDriver traceDriver{dfg, vtxp->user(), aggressive}; + TraceDriver traceDriver{dfg, vtxp->getUser(), aggressive}; // Find the out-of-component driver of the given vertex DfgVertex* const resultp = traceDriver.trace(vtxp, lsb + width - 1, lsb); // Delete unused newly created vertices (these can be created if a @@ -852,7 +859,7 @@ class FixUpSelDrivers final { // Only handle Sel DfgSel* const selp = sink.cast(); if (!selp) return; - // Try to find of the driver of the selected bits outside the cycle + // Try to find the driver of the selected bits outside the cycle DfgVertex* const fixp = TraceDriver::apply(dfg, vtxp, selp->lsb(), selp->width(), false); if (!fixp) return; @@ -968,6 +975,8 @@ class FixUpIndependentRanges final { if (!termp) { AstNodeDType* const dtypep = DfgVertex::dtypeForWidth(width); DfgSel* const selp = new DfgSel{dfg, vtxp->fileline(), dtypep}; + // Same component as 'vtxp', as reads 'vtxp' and will replace 'vtxp' + selp->setUser(vtxp->getUser()); // Do not connect selp->fromp yet, need to do afer replacing 'vtxp' selp->lsb(lsb); termp = selp; @@ -1022,7 +1031,9 @@ class FixUpIndependentRanges final { nImprovements += gatherTerms(termps, dfg, vtxp, indpBits, msb, lsb); } else { // The range is not used, just use constant 0 as a placeholder - termps.emplace_back(new DfgConst{dfg, flp, msb - lsb + 1}); + DfgConst* const constp = new DfgConst{dfg, flp, msb - lsb + 1}; + constp->setUser(0); + termps.emplace_back(constp); } // Next iteration isUsed = !isUsed; @@ -1033,6 +1044,7 @@ class FixUpIndependentRanges final { if (nImprovements) { // Concatenate all the terms to create the replacement DfgVertex* replacementp = termps.front(); + const uint32_t vComp = vtxp->getUser(); for (size_t i = 1; i < termps.size(); ++i) { DfgVertex* const termp = termps[i]; const uint32_t catWidth = replacementp->width() + termp->width(); @@ -1040,6 +1052,14 @@ class FixUpIndependentRanges final { DfgConcat* const catp = new DfgConcat{dfg, flp, dtypep}; catp->rhsp(replacementp); catp->lhsp(termp); + // Need to figure out which component the replacement vertex + // belongs to. If any of the terms are of the original + // component, it's part of that component, otherwise its not + // cyclic (all terms are from outside the original component, + // and feed into the original component). + const uint32_t tComp = termp->getUser(); + const uint32_t rComp = replacementp->getUser(); + catp->setUser(tComp == vComp || rComp == vComp ? vComp : 0); replacementp = catp; } @@ -1071,7 +1091,7 @@ public: // For array variables, fix up element-wise const uint32_t component = varp->getUser(); varp->forEachSink([&](DfgVertex& sink) { - // Ignore if sink is not part of cycle + // Ignore if sink is not part of same cycle if (sink.getUser() != component) return; // Only handle ArraySels with constant index DfgArraySel* const aselp = sink.cast(); diff --git a/test_regress/t/t_dfg_break_cycles.py b/test_regress/t/t_dfg_break_cycles.py index bd94a1b10..f20d6352b 100755 --- a/test_regress/t/t_dfg_break_cycles.py +++ b/test_regress/t/t_dfg_break_cycles.py @@ -88,6 +88,7 @@ test.compile(verilator_flags2=[ "--prefix", "Vopt", "-Werror-UNOPTFLAT", "--dumpi-V3DfgBreakCycles", "9", # To fill code coverage + "--debug", "--debugi", "0", "--dumpi-tree", "0", "-CFLAGS \"-I .. -I ../obj_ref\"", "../obj_ref/Vref__ALL.a", "../../t/" + test.name + ".cpp"