From eaf09ba0e78e0f797130d08ab664325b22587728 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 12 Nov 2022 14:14:32 +0000 Subject: [PATCH] Dfg: resolve multi-driven signal ranges In order to avoid unexpected breakage on multi-driven variables, we resolve in DFG construction by using only the first driver encountered. Also issues the MULTIDRIVEN error for these signals. --- src/V3DfgAstToDfg.cpp | 142 +++++++++++++++---- test_regress/t/t_dfg_multidriver_dfg_bad.out | 39 +++++ test_regress/t/t_dfg_multidriver_dfg_bad.pl | 19 +++ test_regress/t/t_dfg_multidriver_dfg_bad.v | 23 +++ test_regress/t/t_mem_multi_io3.v | 2 + test_regress/t/t_param_type3.v | 15 +- test_regress/t/t_unoptflat_simple_2.v | 2 + test_regress/t/t_unoptflat_simple_2_bad.out | 14 +- 8 files changed, 211 insertions(+), 45 deletions(-) create mode 100644 test_regress/t/t_dfg_multidriver_dfg_bad.out create mode 100755 test_regress/t/t_dfg_multidriver_dfg_bad.pl create mode 100644 test_regress/t/t_dfg_multidriver_dfg_bad.v diff --git a/src/V3DfgAstToDfg.cpp b/src/V3DfgAstToDfg.cpp index b9445d674..a47f7262c 100644 --- a/src/V3DfgAstToDfg.cpp +++ b/src/V3DfgAstToDfg.cpp @@ -79,6 +79,18 @@ class AstToDfgVisitor final : public VNVisitor { // AstNode::user1p // DfgVertex for this AstNode const VNUser1InUse m_user1InUse; + // TYPES + // Represents a driver during canonicalization + struct Driver { + FileLine* m_fileline; + DfgVertex* m_vtxp; + uint32_t m_lsb; + Driver(FileLine* flp, uint32_t lsb, DfgVertex* vtxp) + : m_fileline{flp} + , m_vtxp{vtxp} + , m_lsb{lsb} {} + }; + // STATE DfgGraph* const m_dfgp; // The graph being built @@ -259,6 +271,21 @@ class AstToDfgVisitor final : public VNVisitor { return true; } + // Sometime assignment ranges are coalesced by V3Const, + // so we unpack concatenations for better error reporting. + void addDriver(FileLine* flp, uint32_t lsb, DfgVertex* vtxp, + std::vector& drivers) const { + if (DfgConcat* const concatp = vtxp->cast()) { + DfgVertex* const rhsp = concatp->rhsp(); + addDriver(rhsp->fileline(), lsb, rhsp, drivers); + DfgVertex* const lhsp = concatp->lhsp(); + addDriver(lhsp->fileline(), lsb + rhsp->width(), lhsp, drivers); + concatp->unlinkDelete(*m_dfgp); + } else { + drivers.emplace_back(flp, lsb, vtxp); + } + } + // Canonicalize packed variables void canonicalizePacked() { for (DfgVarPacked* const varp : m_varPackedps) { @@ -270,29 +297,71 @@ class AstToDfgVisitor final : public VNVisitor { } // Gather (and unlink) all drivers - struct Driver { - FileLine* flp; - uint32_t lsb; - DfgVertex* vtxp; - Driver(FileLine* flp, uint32_t lsb, DfgVertex* vtxp) - : flp{flp} - , lsb{lsb} - , vtxp{vtxp} {} - }; std::vector drivers; drivers.reserve(varp->arity()); - varp->forEachSourceEdge([varp, &drivers](DfgEdge& edge, size_t idx) { - UASSERT(edge.sourcep(), "Should not have created undriven sources"); - drivers.emplace_back(varp->driverFileLine(idx), varp->driverLsb(idx), - edge.sourcep()); + varp->forEachSourceEdge([this, varp, &drivers](DfgEdge& edge, size_t idx) { + DfgVertex* const driverp = edge.sourcep(); + UASSERT(driverp, "Should not have created undriven sources"); + addDriver(varp->driverFileLine(idx), varp->driverLsb(idx), driverp, drivers); edge.unlinkSource(); }); - // Sort drivers by LSB - std::stable_sort(drivers.begin(), drivers.end(), - [](const Driver& a, const Driver& b) { return a.lsb < b.lsb; }); + const auto cmp = [](const Driver& a, const Driver& b) { + if (a.m_lsb != b.m_lsb) return a.m_lsb < b.m_lsb; + return a.m_fileline->operatorCompare(*b.m_fileline) < 0; + }; - // TODO: bail on multidriver + // Sort drivers by LSB + std::stable_sort(drivers.begin(), drivers.end(), cmp); + + // Vertices that might have become unused due to multiple driver resolution. Having + // multiple drivers is an error and is hence assumed to be rare, so performance is + // not very important, set will suffice. + std::set prune; + + // Fix multiply driven ranges + for (auto it = drivers.begin(); it != drivers.end();) { + Driver& a = *it++; + const uint32_t aWidth = a.m_vtxp->width(); + const uint32_t aEnd = a.m_lsb + aWidth; + while (it != drivers.end()) { + Driver& b = *it; + // If no overlap, then nothing to do + if (b.m_lsb >= aEnd) break; + + const uint32_t bWidth = b.m_vtxp->width(); + const uint32_t bEnd = b.m_lsb + bWidth; + const uint32_t overlapEnd = std::min(aEnd, bEnd) - 1; + + varp->varp()->v3warn( // + MULTIDRIVEN, + "Bits [" // + << overlapEnd << ":" << b.m_lsb << "] of signal " + << varp->varp()->prettyNameQ() + << " have multiple combinational drivers\n" + << a.m_fileline->warnOther() << "... Location of first driver\n" + << a.m_fileline->warnContextPrimary() << '\n' + << b.m_fileline->warnOther() << "... Location of other driver\n" + << b.m_fileline->warnContextSecondary() << varp->varp()->warnOther() + << "... Only the first driver will be respected"); + + // If the first driver completely covers the range of the second driver, + // we can just delete the second driver completely, otherwise adjust the + // second driver to apply from the end of the range of the first driver. + if (aEnd >= bEnd) { + prune.emplace(b.m_vtxp); + it = drivers.erase(it); + } else { + const auto dtypep = DfgVertex::dtypeForWidth(bEnd - aEnd); + DfgSel* const selp = new DfgSel{*m_dfgp, b.m_vtxp->fileline(), dtypep}; + selp->fromp(b.m_vtxp); + selp->lsb(aEnd - b.m_lsb); + b.m_lsb = aEnd; + b.m_vtxp = selp; + std::stable_sort(it, drivers.end(), cmp); + } + } + } // Coalesce adjacent ranges for (size_t i = 0, j = 1; j < drivers.size(); ++j) { @@ -300,15 +369,15 @@ class AstToDfgVisitor final : public VNVisitor { Driver& b = drivers[j]; // Coalesce adjacent range - const uint32_t aWidth = a.vtxp->width(); - const uint32_t bWidth = b.vtxp->width(); - if (a.lsb + aWidth == b.lsb) { + const uint32_t aWidth = a.m_vtxp->width(); + const uint32_t bWidth = b.m_vtxp->width(); + if (a.m_lsb + aWidth == b.m_lsb) { const auto dtypep = DfgVertex::dtypeForWidth(aWidth + bWidth); - DfgConcat* const concatp = new DfgConcat{*m_dfgp, a.flp, dtypep}; - concatp->rhsp(a.vtxp); - concatp->lhsp(b.vtxp); - a.vtxp = concatp; - b.vtxp = nullptr; // Mark as moved + DfgConcat* const concatp = new DfgConcat{*m_dfgp, a.m_fileline, dtypep}; + concatp->rhsp(a.m_vtxp); + concatp->lhsp(b.m_vtxp); + a.m_vtxp = concatp; + b.m_vtxp = nullptr; // Mark as moved ++m_ctx.m_coalescedAssignments; continue; } @@ -318,17 +387,30 @@ class AstToDfgVisitor final : public VNVisitor { // Compact non-adjacent ranges within the vector if (j != i) { Driver& c = drivers[i]; - UASSERT_OBJ(!c.vtxp, c.flp, "Should have been marked moved"); + UASSERT_OBJ(!c.m_vtxp, c.m_fileline, "Should have been marked moved"); c = b; - b.vtxp = nullptr; // Mark as moved + b.m_vtxp = nullptr; // Mark as moved } } - // Reinsert sources in order + // Reinsert drivers in order varp->resetSources(); for (const Driver& driver : drivers) { - if (!driver.vtxp) break; // Stop at end of cmpacted list - varp->addDriver(driver.flp, driver.lsb, driver.vtxp); + if (!driver.m_vtxp) break; // Stop at end of compacted list + varp->addDriver(driver.m_fileline, driver.m_lsb, driver.m_vtxp); + } + + // Prune vertices potentially unused due to resolving multiple drivers. + while (!prune.empty()) { + // Pop last vertex + const auto it = prune.begin(); + DfgVertex* const vtxp = *it; + prune.erase(it); + // If used (or a variable), then done + if (vtxp->hasSinks() || vtxp->is()) continue; + // If unused, then add sources to work list and delete + vtxp->forEachSource([&](DfgVertex& src) { prune.emplace(&src); }); + vtxp->unlinkDelete(*m_dfgp); } } } diff --git a/test_regress/t/t_dfg_multidriver_dfg_bad.out b/test_regress/t/t_dfg_multidriver_dfg_bad.out new file mode 100644 index 000000000..ce36240b8 --- /dev/null +++ b/test_regress/t/t_dfg_multidriver_dfg_bad.out @@ -0,0 +1,39 @@ +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:13:18: Bits [3:1] of signal 'a' have multiple combinational drivers + : ... In instance t + t/t_dfg_multidriver_dfg_bad.v:14:19: ... Location of first driver + 14 | assign a[3:0] = i[3:0]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:15:19: ... Location of other driver + 15 | assign a[4:1] = ~i[4:1]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:13:18: ... Only the first driver will be respected + ... For warning description see https://verilator.org/warn/MULTIDRIVEN?v=latest + ... Use "/* verilator lint_off MULTIDRIVEN */" and lint_on around source to disable this message. +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:13:18: Bits [3:3] of signal 'a' have multiple combinational drivers + : ... In instance t + t/t_dfg_multidriver_dfg_bad.v:14:19: ... Location of first driver + 14 | assign a[3:0] = i[3:0]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:16:17: ... Location of other driver + 16 | assign a[3] = ~i[3]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:13:18: ... Only the first driver will be respected +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:13:18: Bits [7:6] of signal 'a' have multiple combinational drivers + : ... In instance t + t/t_dfg_multidriver_dfg_bad.v:17:19: ... Location of first driver + 17 | assign a[8:5] = i[8:5]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:18:19: ... Location of other driver + 18 | assign a[7:6] = ~i[7:6]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:13:18: ... Only the first driver will be respected +%Warning-MULTIDRIVEN: t/t_dfg_multidriver_dfg_bad.v:13:18: Bits [9:9] of signal 'a' have multiple combinational drivers + : ... In instance t + t/t_dfg_multidriver_dfg_bad.v:19:17: ... Location of first driver + 19 | assign a[9] = i[9]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:20:19: ... Location of other driver + 20 | assign a[9] = ~i[9]; + | ^ + t/t_dfg_multidriver_dfg_bad.v:13:18: ... Only the first driver will be respected +%Error: Exiting due to diff --git a/test_regress/t/t_dfg_multidriver_dfg_bad.pl b/test_regress/t/t_dfg_multidriver_dfg_bad.pl new file mode 100755 index 000000000..eae347a52 --- /dev/null +++ b/test_regress/t/t_dfg_multidriver_dfg_bad.pl @@ -0,0 +1,19 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2022 by Geza Lore. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(vlt => 1); + +compile( + fails => 1, + expect_filename => $Self->{golden_filename}, +); + +ok(1); +1; diff --git a/test_regress/t/t_dfg_multidriver_dfg_bad.v b/test_regress/t/t_dfg_multidriver_dfg_bad.v new file mode 100644 index 000000000..724b33539 --- /dev/null +++ b/test_regress/t/t_dfg_multidriver_dfg_bad.v @@ -0,0 +1,23 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Geza Lore. +// SPDX-License-Identifier: CC0-1.0 + +`default_nettype none + +module t( + input wire [10:0] i, + output wire [10:0] o +); + logic [10:0] a; + assign a[3:0] = i[3:0]; + assign a[4:1] = ~i[4:1]; + assign a[3] = ~i[3]; + assign a[8:5] = i[8:5]; + assign a[7:6] = ~i[7:6]; + assign a[9] = i[9]; + assign a[9] = ~i[9]; + assign a[10] = i[10]; + assign o = a; +endmodule diff --git a/test_regress/t/t_mem_multi_io3.v b/test_regress/t/t_mem_multi_io3.v index 1e4d7434d..3a70d6e26 100644 --- a/test_regress/t/t_mem_multi_io3.v +++ b/test_regress/t/t_mem_multi_io3.v @@ -40,7 +40,9 @@ module testio input logic signed [3:0] [3:0] [8:0] arr3d_in, output logic signed [3:0] [35:0] arr2d_out ); + /* verilator lint_off MULTIDRIVEN */ logic signed [3:0] [35:0] ar2d_out_pre; + /* verilator lint_on MULTIDRIVEN */ always_comb ar2d_out_pre[0][35:0] = {arr3d_in[0][0][8:0], arr3d_in[0][1][8:0], arr3d_in[0][2][8:0], arr3d_in[0][3][8:0]}; always_comb ar2d_out_pre[0][35:0] = {arr3d_in[0][0][8:0], arr3d_in[0][1][8:0], arr3d_in[0][2][8:0], arr3d_in[0][3][8:0]}; diff --git a/test_regress/t/t_param_type3.v b/test_regress/t/t_param_type3.v index dca6bdc19..cbce4c24b 100644 --- a/test_regress/t/t_param_type3.v +++ b/test_regress/t/t_param_type3.v @@ -8,26 +8,25 @@ typedef logic T_t; module t (/*AUTOARG*/ // Outputs - o, o2, + o, ob, o2, o2b, // Inputs i ); input T_t i; - output T_t o; - output T_t o2; + output T_t o, ob, o2, o2b; sub1 #(.T_t(T_t), .CHECK(1)) - sub1 (.i, .o); + sub1 (.i, .o(o)); sub2 #(.T_t(T_t), .CHECK(2)) sub2 (.i, .o(o2)); sub1 #(T_t, 1) - sub1b (i, o); + sub1b (i, ob); sub2 #(T_t, 2) - sub2b (i, o2); + sub2b (i, o2b); endmodule @@ -35,8 +34,8 @@ module sub1 (i,o); parameter type T_t = real; localparam type T2_t = T_t; parameter int CHECK = 0; - input T_t i; - output T2_t o; + input T_t i; + output T2_t o; assign o = i; if (CHECK != 1) $error; endmodule diff --git a/test_regress/t/t_unoptflat_simple_2.v b/test_regress/t/t_unoptflat_simple_2.v index 549f69b43..b455b1469 100644 --- a/test_regress/t/t_unoptflat_simple_2.v +++ b/test_regress/t/t_unoptflat_simple_2.v @@ -12,7 +12,9 @@ module t (/*AUTOARG*/ ); input clk; + /* verilator lint_off MULTIDRIVEN */ wire [2:0] x; + /* verilator lint_on MULTIDRIVEN */ assign x[1:0] = { x[0], clk }; assign x[2:1] = x[1:0]; diff --git a/test_regress/t/t_unoptflat_simple_2_bad.out b/test_regress/t/t_unoptflat_simple_2_bad.out index 6ec899e69..be3386782 100644 --- a/test_regress/t/t_unoptflat_simple_2_bad.out +++ b/test_regress/t/t_unoptflat_simple_2_bad.out @@ -1,14 +1,14 @@ -%Warning-UNOPTFLAT: t/t_unoptflat_simple_2.v:15:15: Signal unoptimizable: Circular combinational logic: 't.x' - 15 | wire [2:0] x; +%Warning-UNOPTFLAT: t/t_unoptflat_simple_2.v:16:15: Signal unoptimizable: Circular combinational logic: 't.x' + 16 | wire [2:0] x; | ^ ... For warning description see https://verilator.org/warn/UNOPTFLAT?v=latest ... Use "/* verilator lint_off UNOPTFLAT */" and lint_on around source to disable this message. - t/t_unoptflat_simple_2.v:15:15: Example path: t.x - t/t_unoptflat_simple_2.v:17:18: Example path: ASSIGNW - t/t_unoptflat_simple_2.v:15:15: Example path: t.x + t/t_unoptflat_simple_2.v:16:15: Example path: t.x + t/t_unoptflat_simple_2.v:13:10: Example path: ASSIGNW + t/t_unoptflat_simple_2.v:16:15: Example path: t.x ... Widest variables candidate to splitting: - t/t_unoptflat_simple_2.v:15:15: t.x, width 3, circular fanout 2, can split_var + t/t_unoptflat_simple_2.v:16:15: t.x, width 3, circular fanout 1, can split_var ... Candidates with the highest fanout: - t/t_unoptflat_simple_2.v:15:15: t.x, width 3, circular fanout 2, can split_var + t/t_unoptflat_simple_2.v:16:15: t.x, width 3, circular fanout 1, can split_var ... Suggest add /*verilator split_var*/ to appropriate variables above. %Error: Exiting due to