From d637b665cede14a953260c84511467ab304309a8 Mon Sep 17 00:00:00 2001 From: Patrick Creighton Date: Tue, 22 Sep 2026 09:46:38 -0700 Subject: [PATCH] Synthesize successive whole-element array assignments (#8316) Preserve previous whole-element drivers when synthesizing successive constant-index assignments to unpacked arrays. Retain earlier values through their assignment temporaries and keep the default while some elements remain uncovered. Complete the array-default tracing path exposed by this representation. Keep public-write checks, control-flow restrictions, and scheduling unchanged, and cover propagation and conservative fallbacks in the existing DFG regressions. Part of #7964; array-only follow-up to #7966. --- src/V3DfgBreakCycles.cpp | 38 ++---- src/V3DfgSynthesize.cpp | 78 ++++++++++-- test_regress/t/t_dfg_break_cycles.v | 11 ++ test_regress/t/t_dfg_multidriver_dfg_bad.out | 25 ++-- test_regress/t/t_dfg_multidriver_dfg_bad.v | 13 +- test_regress/t/t_dfg_synthesis.v | 124 +++++++++++++++++++ 6 files changed, 240 insertions(+), 49 deletions(-) diff --git a/src/V3DfgBreakCycles.cpp b/src/V3DfgBreakCycles.cpp index 6e2760456..6c4366470 100644 --- a/src/V3DfgBreakCycles.cpp +++ b/src/V3DfgBreakCycles.cpp @@ -567,7 +567,7 @@ class TraceDriver final : public DfgVisitor { void visit(DfgSpliceArray* vtxp) override { UASSERT_OBJ(m_splicep == vtxp, vtxp, "Unexpected trace of DfgSpliceArray"); - // DfgVertex* defaultp = m_defaultp; + DfgVertex* const defaultp = m_defaultp; m_defaultp = nullptr; m_splicep = nullptr; @@ -575,34 +575,17 @@ class TraceDriver final : public DfgVisitor { const uint32_t idx = m_idxs.back(); if (DfgVertex* const driverp = vtxp->driverAt(idx)) { DfgVertex* const srcp = driverp->as()->srcp(); - // TODO: this is unreachable today, but with e.g. #8316 it wouldn't be - // // TODO: replace DfgSplice with DfgInsert modeling - // // Annoying corner case: If the element itself is a splice, we need - // // a defaultp for that element if there was one for the whole array. - // // Make one up by selecting out of the default. It will be removed - // // later if unused. Pretend it's in the same component as the array - // // default as trace needs to continue in that case. - // if (defaultp && srcp->is()) { - // DfgArraySel* const aselp = new DfgArraySel{m_dfg, - // vtxp->fileline(), srcp->dtype()}; - // m_sccInfo.add(*aselp, m_sccInfo.get(*defaultp)); - // DfgConst* const idxp = make(vtxp, 32); - // idxp->num().setLong(idx); - // aselp->fromp(defaultp); - // aselp->bitp(idxp); - // m_defaultp = aselp; - // m_splicep = srcp; - // } + if (srcp->is()) { + // Partial-element propagation is rejected during synthesis. + UASSERT_OBJ(!defaultp, vtxp, "Array default with partial element driver"); + m_splicep = srcp; + } // Consume this index, then trace the element value - if (srcp->is()) m_splicep = srcp; RETURN_RESULT(tracePopIdx(srcp)); } - // TODO: this is unreachable, as syntheis can't create it today. - // // Element not driven explicitly, so it comes from the default array. Keep the - // // index pending (the default is the whole array, indexed the same way) and - // // continue tracing it. - // UASSERT_OBJ(m_defaultp, vtxp, "Independent array element should have a driver or - // default"); RETURN RESULT(traceSameIdx(m_defaultp)); + // An element not driven explicitly comes from the default array at the same index. + UASSERT_OBJ(defaultp, vtxp, "Independent array element should have a driver or default"); + RETURN_RESULT(traceSameIdx(defaultp)); } void visit(DfgVertexVar* vtxp) override { @@ -612,9 +595,6 @@ class TraceDriver final : public DfgVisitor { DfgVertex* const drvp = srcp ? srcp : defaultp; // If we are about to trace a splice, set the defaultp to the corresponding default if (srcp && srcp->is()) { - // Unreachable today: getting an array into a fixable cycle needs multiple - // assignments in a process, which V3DfgSynthesize rejects ("Can't do arrays yet"). - UASSERT_OBJ(!defaultp || vtxp->isPacked(), vtxp, "Array variable with defaultp"); m_defaultp = defaultp; m_splicep = srcp; } diff --git a/src/V3DfgSynthesize.cpp b/src/V3DfgSynthesize.cpp index 706b1fa29..cdbe05814 100644 --- a/src/V3DfgSynthesize.cpp +++ b/src/V3DfgSynthesize.cpp @@ -701,6 +701,28 @@ class AstToDfgSynthesize final { return drivers; } + // Returns true if the driver supplies a complete array element. + static bool driverCoversWholeElement(const Driver& driver) { + const DfgUnitArray* const unitp = driver.m_vtxp->cast(); + if (!unitp) return false; + if (DfgVertexSplice* const splicep = unitp->srcp()->cast()) { + return splicep->wholep(); + } + return true; + } + + // Unlike 'wholep', several element drivers can collectively cover an array. + static bool spliceCoversWhole(DfgVertexSplice* const splicep) { + if (splicep->wholep()) return true; + if (splicep->isPacked()) return false; + uint32_t next = 0; + for (const Driver& driver : gatherDrivers(splicep)) { + if (driver.m_lo != next || !driverCoversWholeElement(driver)) return false; + next = driver.m_hi + 1; + } + return next == splicep->size(); + } + // Returns true if the driver cone contains any variable introduced by // tristate lowering. Used to distinguish intentional tristate contributor // overlap from accidental multidrive. @@ -1313,6 +1335,42 @@ class AstToDfgSynthesize final { return propagatedDrivers; } + // Propagate whole elements with a linear walk over the sorted driver lists. + // Reads use the previous value temporary, preserving assignment-version bindings. + // Partial elements and non-unit drivers retain the nonsynthesized-process fallback. + bool computePropagatedArrayDrivers(const std::vector& newDrivers, + DfgVertexVar* const oldp, + std::vector& propagatedDrivers) { + // Bound quadratic vertex growth: array drivers cannot be coalesced. + static constexpr uint32_t MAX_ARRAY_ELEMENTS = 32; + if (oldp->size() > MAX_ARRAY_ELEMENTS) return false; + for (const Driver& driver : newDrivers) { + if (!driverCoversWholeElement(driver)) return false; + } + const std::vector oldDrivers = gatherDrivers(oldp->srcp()->as()); + UASSERT_OBJ(!oldDrivers.empty(), oldp, "Should have a proper driver"); + for (const Driver& driver : oldDrivers) { + if (!driverCoversWholeElement(driver)) return false; + } + + propagatedDrivers.reserve(oldDrivers.size()); + auto nIt = newDrivers.begin(); + for (const Driver& oDriver : oldDrivers) { + while (nIt != newDrivers.end() && nIt->m_lo < oDriver.m_lo) ++nIt; + if (nIt != newDrivers.end() && nIt->m_lo == oDriver.m_lo) continue; + + FileLine* const flp = oDriver.m_flp; + const DfgUnitArray* const oldUnitp = oDriver.m_vtxp->as(); + DfgArraySel* const selp = make(flp, oldUnitp->srcp()->dtype()); + selp->fromp(oldp); + selp->bitp(make(flp, static_cast(VL_IDATASIZE), oDriver.m_lo)); + DfgUnitArray* const newUnitp = make(flp, oldUnitp->dtype()); + newUnitp->srcp(selp); + propagatedDrivers.emplace_back(newUnitp, oDriver.m_lo, flp); + } + return true; + } + // Given the drivers of a variable after converting a single statement // 'newp', add drivers from 'oldp' that were not reassigned be drivers // in newp. This computes the total result of all previous assignments. @@ -1328,25 +1386,25 @@ class AstToDfgSynthesize final { // If the old value is the real variable we just computed the new value for, // then it is the circular feedback into the synthesized block, add it as default driver. if (oldp->vscp() == vscp) { - if (!nSplicep->wholep()) newp->defaultp(oldp); + if (!spliceCoversWhole(nSplicep)) newp->defaultp(oldp); return true; } UASSERT_OBJ(oldp->srcp(), vscp, "Previously assigned variable has no driver"); - // Can't do arrays yet - if (!newp->isPacked()) { - ++m_ctx.m_synt.nonSynArray; - return false; - } - // Gather drivers of 'newp' - they are in incresing range order with no overlaps UASSERT_OBJ(!newp->defaultp(), newp, "Converted value should not have default"); std::vector nDrivers = gatherDrivers(newp->srcp()->as()); UASSERT_OBJ(!nDrivers.empty(), newp, "Should have a proper driver"); // Additional drivers of 'newp' propagated from 'oldp' - std::vector pDrivers = computePropagatedDrivers(nDrivers, oldp); + std::vector pDrivers; + if (newp->isPacked()) { + pDrivers = computePropagatedDrivers(nDrivers, oldp); + } else if (!computePropagatedArrayDrivers(nDrivers, oldp, pDrivers)) { + ++m_ctx.m_synt.nonSynArray; + return false; + } if (!pDrivers.empty()) { // Need to merge propagated sources, so reset the splice @@ -1357,13 +1415,13 @@ class AstToDfgSynthesize final { std::merge(nDrivers.begin(), nDrivers.end(), pDrivers.begin(), pDrivers.end(), std::back_inserter(drivers)); // Coalesce adjacent ranges - coalesceDrivers(drivers); + if (newp->isPacked()) coalesceDrivers(drivers); // Reinsert drivers in order for (const Driver& d : drivers) nSplicep->addDriver(d.m_vtxp, d.m_lo, d.m_flp); } // If the old had a default, add to the new one too, unless redundant - if (oldp->defaultp() && !nSplicep->wholep()) newp->defaultp(oldp->defaultp()); + if (oldp->defaultp() && !spliceCoversWhole(nSplicep)) newp->defaultp(oldp->defaultp()); // Done return true; diff --git a/test_regress/t/t_dfg_break_cycles.v b/test_regress/t/t_dfg_break_cycles.v index 83ff1911c..84503f794 100644 --- a/test_regress/t/t_dfg_break_cycles.v +++ b/test_regress/t/t_dfg_break_cycles.v @@ -251,6 +251,17 @@ module t ( `signal(ARRAY_3, 3); // UNOPTFLAT assign ARRAY_3 = array_3[0]; + logic [6:0] array_default[3]; // UNOPTFLAT + logic [6:0] array_default_in; + assign array_default[1] = rand_a[6:0]; + always @* begin + array_default_in = array_default[1]; + array_default[0] = rand_b[6:0] ^ array_default_in; + array_default[2] = array_default[1]; + end + `signal(ARRAY_DEFAULT, 21); + assign ARRAY_DEFAULT = {array_default[2], array_default[1], array_default[0]}; + `signal(ADD_A, 8); // UNOPTFLAT `signal(ADD_B, 8); `signal(ADD_C, 8); diff --git a/test_regress/t/t_dfg_multidriver_dfg_bad.out b/test_regress/t/t_dfg_multidriver_dfg_bad.out index feec4ce32..58f54ce3e 100644 --- a/test_regress/t/t_dfg_multidriver_dfg_bad.out +++ b/test_regress/t/t_dfg_multidriver_dfg_bad.out @@ -84,27 +84,34 @@ t/t_dfg_multidriver_dfg_bad.v:60:18: ... Location of offending driver 60 | assign z[10:7] = i[10:7]; | ^ -%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:76:16: Bits [5:2] of signal 't.sub_1.a' have multiple combinational drivers. This can cause performance degradation. +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:87:16: Bits [5:2] of signal 't.sub_1.a' have multiple combinational drivers. This can cause performance degradation. t/t_dfg_multidriver_dfg_bad.v:63:18: ... Location of offending driver 63 | assign sub_1.a = i; | ^ - t/t_dfg_multidriver_dfg_bad.v:77:17: ... Location of offending driver - 77 | assign a[5:2] = i[5:2]; + t/t_dfg_multidriver_dfg_bad.v:88:17: ... Location of offending driver + 88 | assign a[5:2] = i[5:2]; | ^ -%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:76:16: Bits [3:2] of signal 't.sub_2.a' have multiple combinational drivers. This can cause performance degradation. +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:87:16: Bits [3:2] of signal 't.sub_2.a' have multiple combinational drivers. This can cause performance degradation. t/t_dfg_multidriver_dfg_bad.v:67:23: ... Location of offending driver 67 | assign sub_2.a[3:0] = i[3:0]; | ^ - t/t_dfg_multidriver_dfg_bad.v:77:17: ... Location of offending driver - 77 | assign a[5:2] = i[5:2]; + t/t_dfg_multidriver_dfg_bad.v:88:17: ... Location of offending driver + 88 | assign a[5:2] = i[5:2]; | ^ -%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:76:16: Bit [5] of signal 't.sub_2.a' have multiple combinational drivers. This can cause performance degradation. - t/t_dfg_multidriver_dfg_bad.v:77:17: ... Location of offending driver - 77 | assign a[5:2] = i[5:2]; +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:87:16: Bit [5] of signal 't.sub_2.a' have multiple combinational drivers. This can cause performance degradation. + t/t_dfg_multidriver_dfg_bad.v:88:17: ... Location of offending driver + 88 | assign a[5:2] = i[5:2]; | ^ t/t_dfg_multidriver_dfg_bad.v:66:24: ... Location of offending driver 66 | assign sub_2.a[10:5] = i[10:5]; | ^ +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:69:16: Element [0] of signal 't.array_always' have multiple combinational drivers. This can cause performance degradation. + t/t_dfg_multidriver_dfg_bad.v:71:17: ... Location of offending driver + 71 | array_always[0] = i; + | ^ + t/t_dfg_multidriver_dfg_bad.v:75:17: ... Location of offending driver + 75 | array_always[0] = k[0]; + | ^ %Error: Internal Error: t/t_dfg_multidriver_dfg_bad.v:48:12: ../V3Gate.cpp:#: Concat on LHS of assignment; V3Const should have deleted it 48 | {y[1:0], y[2:1]} = i[3:0] + 4'd5; | ^ diff --git a/test_regress/t/t_dfg_multidriver_dfg_bad.v b/test_regress/t/t_dfg_multidriver_dfg_bad.v index 39ceec7e9..36cfe8821 100644 --- a/test_regress/t/t_dfg_multidriver_dfg_bad.v +++ b/test_regress/t/t_dfg_multidriver_dfg_bad.v @@ -66,7 +66,18 @@ module t ( assign sub_2.a[10:5] = i[10:5]; assign sub_2.a[3:0] = i[3:0]; - assign o = a ^ u[3] ^ v[3] ^ w[3] ^ x[3] ^ y ^ z ^ sub_1.a ^ sub_2.a; + logic [10:0] array_always[3]; + always_comb begin + array_always[0] = i; + array_always[1] = j[1]; + end + always_comb begin + array_always[0] = k[0]; + array_always[2] = j[2]; + end + + assign o = a ^ u[3] ^ v[3] ^ w[3] ^ x[3] ^ y ^ z ^ sub_1.a ^ sub_2.a + ^ array_always[0] ^ array_always[1] ^ array_always[2]; endmodule diff --git a/test_regress/t/t_dfg_synthesis.v b/test_regress/t/t_dfg_synthesis.v index 27995571e..014b1b27c 100644 --- a/test_regress/t/t_dfg_synthesis.v +++ b/test_regress/t/t_dfg_synthesis.v @@ -628,4 +628,128 @@ module t ( end `signal(ARRAY_READ, array_read); + logic array_ready; + logic array_transfer_ready; + logic [1:0] array_status[2]; + always_comb begin + array_ready = rand_a[0]; + array_transfer_ready = array_ready && rand_b[0]; + array_status[0] = {array_ready, array_transfer_ready}; + array_status[1] = {array_transfer_ready, array_ready}; + end + `signal(ARRAY_SEQUENTIAL, {array_status[1], array_status[0]}); + + typedef logic [6:0] array_element_t; + array_element_t array_descending[3:1]; + array_element_t array_ascending[1:3]; + always_comb begin + array_descending[2] = rand_a[6:0]; + array_descending[3] = rand_b[6:0]; + array_descending[1] = rand_a[13:7]; + array_ascending[2] = rand_b[6:0]; + array_ascending[1] = rand_a[6:0]; + array_ascending[3] = rand_b[13:7]; + end + `signal(ARRAY_BOUNDS, {array_descending[1], array_descending[2], array_descending[3], + array_ascending[1], array_ascending[2], array_ascending[3]}); + + logic [6:0] array_intermediate[2]; + logic [6:0] array_before; + logic [6:0] array_after; + always_comb begin + array_intermediate[0] = rand_a[6:0]; + array_before = array_intermediate[0]; + array_intermediate[1] = rand_b[6:0]; + array_intermediate[0] = ~rand_a[6:0]; + array_after = array_intermediate[0]; + array_intermediate[1] = array_after ^ rand_b[6:0]; + end + `signal(ARRAY_INTERMEDIATE, {array_intermediate[1], array_intermediate[0], array_before, + array_after}); + + logic [6:0] array_retained[4]; + logic [6:0] array_retained_before; + assign array_retained[1] = rand_b[6:0]; + always @* begin + array_retained_before = array_retained[1]; + {array_retained[3], array_retained[0]} = {rand_a[6:0], rand_a[13:7]}; + array_retained[2] = array_retained[1] ^ rand_a[20:14]; + {array_retained[3], array_retained[0]} = {array_retained_before ^ rand_a[6:0], ~rand_a[6:0]}; + end + `signal(ARRAY_RETAINED, {array_retained[3], array_retained[2], array_retained[1], + array_retained[0], array_retained_before}); + + logic [6:0] array_disjoint[4]; + always_comb begin + array_disjoint[3] = rand_a[6:0]; + array_disjoint[1] = rand_b[6:0]; + end + always_comb begin + array_disjoint[0] = rand_a[13:7]; + array_disjoint[2] = rand_b[13:7]; + end + `signal(ARRAY_DISJOINT, {array_disjoint[3], array_disjoint[2], array_disjoint[1], + array_disjoint[0]}); + + // verilator lint_off MULTIDRIVEN + // Both blocks write the same value to element 0 so the equivalence check + // does not depend on their execution order. + logic [6:0] array_multidriven[3]; + always_comb begin // revert + array_multidriven[0] = rand_a[6:0]; + array_multidriven[1] = rand_b[6:0]; + end + always_comb begin // revert + array_multidriven[0] = rand_a[6:0]; + array_multidriven[2] = rand_a[13:7]; + end + // verilator lint_on MULTIDRIVEN + `signal(ARRAY_MULTIDRIVEN, {array_multidriven[2], array_multidriven[1], array_multidriven[0]}); + + logic [6:0] array_at_limit[32]; + always_comb begin + /*verilator unroll_full*/ + for (int k = 0; k < 32; ++k) array_at_limit[k] = 7'(rand_a >> k) ^ 7'(k); + end + `signal(ARRAY_AT_LIMIT, {array_at_limit[31], array_at_limit[16], array_at_limit[0]}); + + logic [6:0] array_large[33]; + always_comb begin // nosynth + /*verilator unroll_full*/ + for (int k = 0; k < 33; ++k) array_large[k] = 7'(rand_a >> k) ^ 7'(k); + end + `signal(ARRAY_LARGE, {array_large[32], array_large[16], array_large[0]}); + + logic [6:0] array_final_read[2]; + logic [6:0] array_final_value; + always_comb begin // nosynth + array_final_read[0] = rand_a[6:0]; + array_final_read[1] = rand_b[6:0]; + array_final_value = array_final_read[0]; + end + `signal(ARRAY_FINAL_READ, {array_final_read[1], array_final_read[0], array_final_value}); + + logic [6:0] array_partial_reassign[2]; + always_comb begin // nosynth + array_partial_reassign[0] = rand_a[6:0]; + array_partial_reassign[1] = rand_b[6:0]; + array_partial_reassign[1][2:0] = rand_a[2:0]; + end + `signal(ARRAY_PARTIAL_REASSIGN, {array_partial_reassign[1], array_partial_reassign[0]}); + + logic [6:0] array_partial_previous[2]; + always_comb begin // nosynth + array_partial_previous[0][2:0] = rand_a[2:0]; + array_partial_previous[1] = rand_b[6:0]; + array_partial_previous[0][6:3] = rand_a[6:3]; + end + `signal(ARRAY_PARTIAL_PREVIOUS, {array_partial_previous[1], array_partial_previous[0]}); + + logic [6:0] array_whole_previous[2]; + always_comb begin // nosynth + array_whole_previous = array_intermediate; + array_whole_previous[1] = rand_a[6:0]; + end + `signal(ARRAY_WHOLE_PREVIOUS, {array_whole_previous[1], array_whole_previous[0]}); + endmodule