From bb2822f4b59326bb5147cfc2934df70cadce7b6c Mon Sep 17 00:00:00 2001 From: Jeremy Bennett Date: Tue, 26 Feb 2013 22:26:47 -0500 Subject: [PATCH] Add --report-unoptflat, bug611. Signed-off-by: Wilson Snyder --- Changes | 2 + bin/verilator | 27 +++++ src/V3Graph.cpp | 5 + src/V3Graph.h | 7 +- src/V3GraphAlg.cpp | 56 +++++++++- src/V3Options.cpp | 2 + src/V3Options.h | 2 + src/V3Order.cpp | 118 ++++++++++++++++++++- src/V3OrderGraph.h | 15 ++- test_regress/t/t_unoptflat_simple.v | 32 ++++++ test_regress/t/t_unoptflat_simple_2.v | 35 ++++++ test_regress/t/t_unoptflat_simple_2_bad.pl | 24 +++++ test_regress/t/t_unoptflat_simple_3.v | 79 ++++++++++++++ test_regress/t/t_unoptflat_simple_3_bad.pl | 18 ++++ test_regress/t/t_unoptflat_simple_bad.pl | 18 ++++ 15 files changed, 434 insertions(+), 6 deletions(-) create mode 100644 test_regress/t/t_unoptflat_simple.v create mode 100644 test_regress/t/t_unoptflat_simple_2.v create mode 100755 test_regress/t/t_unoptflat_simple_2_bad.pl create mode 100644 test_regress/t/t_unoptflat_simple_3.v create mode 100755 test_regress/t/t_unoptflat_simple_3_bad.pl create mode 100755 test_regress/t/t_unoptflat_simple_bad.pl diff --git a/Changes b/Changes index b3dc5af5d..350478b8f 100644 --- a/Changes +++ b/Changes @@ -5,6 +5,8 @@ indicates the contributor was also the author of the fix; Thanks! * Verilator 3.846-devel +*** Add --report-unoptflat, bug611. [Jeremy Bennett] + *** Add duplicate clock gate optimization, msg980. [Varun Koyyalagunta] Disabled unless -OD or -O3 used, please try it as may get some significant speedups. diff --git a/bin/verilator b/bin/verilator index ca24c7910..6c11a0a11 100755 --- a/bin/verilator +++ b/bin/verilator @@ -308,6 +308,7 @@ descriptions in the next sections for more information. --private Debugging; see docs --psl Enable PSL parsing --public Debugging; see docs + --report-unoptflat Extra diagnostics for UNOPTFLAT --savable Enable model save-restore --sc Create SystemC output --sp Create SystemPerl output @@ -877,6 +878,27 @@ inlining. This will also turn off inlining as if all modules had a /*verilator public_module*/, unless the module specifically enabled it with /*verilator inline_module*/. +=item --report-unoptflat + +Extra diagnostics for UNOPTFLAT warnings. This includes for each loop, the +10 widest variables in the loop, and the 10 most fanned out variables in +the loop. These are candidates for splitting into multiple variables to +break the loop. + +In addition produces a GraphViz DOT file of the entire strongly connected +components within the source associated with each loop. This is produced +irrespective of whether --dump-tree is set. Such graphs may help in +analysing the problem, but can be very large indeed. + +Various commands exist for viewing and manipulating DOT files. For example +the I command can be used to convert a DOT file to a PDF for +printing. For example: + + dot -Tpdf -O Vt_unoptflat_simple_2_35_unoptflat.dot + +will generate a PDF Vt_unoptflat_simple_2_35_unoptflat.dot.pdf from the DOT +file. + =item --savable Enable including save and restore functions in the generated model. @@ -3146,6 +3168,11 @@ The UNOPTFLAT warning may also occur where outputs from a block of logic are independent, but occur in the same always block. To fix this, use the isolate_assignments meta comment described above. +To assist in resolving UNOPTFLAT, the option C<--report-unoptflat> can be +used, which will provide suggestions for variables that can be split up, +and a graph of all the nodes connected in the loop. See the L +section for more details. + Ignoring this warning will only slow simulations, it will simulate correctly. diff --git a/src/V3Graph.cpp b/src/V3Graph.cpp index 5496b112b..f67f82e17 100644 --- a/src/V3Graph.cpp +++ b/src/V3Graph.cpp @@ -280,6 +280,11 @@ void V3Graph::dumpDotFilePrefixed(const string& nameComment, bool colorAsSubgrap } } +//! Variant of dumpDotFilePrefixed without --dump option check +void V3Graph::dumpDotFilePrefixedAlways(const string& nameComment, bool colorAsSubgraph) { + dumpDotFile(v3Global.debugFilename(nameComment)+".dot", colorAsSubgraph); +} + void V3Graph::dumpDotFile(const string& filename, bool colorAsSubgraph) { // This generates a file used by graphviz, http://www.graphviz.org // "hardcoded" parameters: diff --git a/src/V3Graph.h b/src/V3Graph.h index 4fbd93866..bbf332df5 100644 --- a/src/V3Graph.h +++ b/src/V3Graph.h @@ -119,13 +119,17 @@ public: /// Remove any redundant edges, weights become SUM of any other weight void removeRedundantEdgesSum(V3EdgeFuncP edgeFuncp); - /// Call loopsVertexCb on any loops starting where specified + /// Call loopsVertexCb on any one loop starting where specified void reportLoops(V3EdgeFuncP edgeFuncp, V3GraphVertex* vertexp); + /// Build a subgraph of all loops starting where specified + void subtreeLoops(V3EdgeFuncP edgeFuncp, V3GraphVertex* vertexp, V3Graph* loopGraphp); + /// Debugging void dump(ostream& os=cout); void dumpDotFile(const string& filename, bool colorAsSubgraph); void dumpDotFilePrefixed(const string& nameComment, bool colorAsSubgraph=false); + void dumpDotFilePrefixedAlways(const string& nameComment, bool colorAsSubgraph=false); void userClearVertices(); void userClearEdges(); static void test(); @@ -170,6 +174,7 @@ public: virtual ~V3GraphVertex() {} void unlinkEdges(V3Graph* graphp); void unlinkDelete(V3Graph* graphp); + // ACCESSORS virtual string name() const { return ""; } virtual string dotColor() const { return "black"; } diff --git a/src/V3GraphAlg.cpp b/src/V3GraphAlg.cpp index 6d2303ada..161017bc9 100644 --- a/src/V3GraphAlg.cpp +++ b/src/V3GraphAlg.cpp @@ -376,6 +376,58 @@ void V3Graph::reportLoops(V3EdgeFuncP edgeFuncp, V3GraphVertex* vertexp) { GraphAlgRLoops (this, edgeFuncp, vertexp); } + +//###################################################################### +//###################################################################### +// Algorithms - subtrees + +class GraphAlgSubtrees : GraphAlg { +private: + V3Graph* m_loopGraphp; + + //! Iterate through all connected nodes of a graph with a loop or loops. + V3GraphVertex* vertexIterateAll(V3GraphVertex* vertexp) { + if (V3GraphVertex* newVertexp = (V3GraphVertex*)vertexp->userp()) { + return newVertexp; + } else { + newVertexp = vertexp->clone(m_loopGraphp); + vertexp->userp(newVertexp); + + for (V3GraphEdge* edgep = vertexp->outBeginp(); + edgep; edgep=edgep->outNextp()) { + if (followEdge(edgep)) { + V3GraphEdge* newEdgep = (V3GraphEdge*)edgep->userp(); + if (!newEdgep) { + V3GraphVertex* newTop = vertexIterateAll(edgep->top()); + newEdgep = edgep->clone(m_loopGraphp, newVertexp, + newTop); + edgep->userp(newEdgep); + } + } + } + return newVertexp; + } + } + +public: + GraphAlgSubtrees(V3Graph* graphp, V3Graph* loopGraphp, + V3EdgeFuncP edgeFuncp, V3GraphVertex* vertexp) + : GraphAlg(graphp, edgeFuncp), m_loopGraphp (loopGraphp) { + // Vertex::m_userp - New vertex if we have seen this vertex already + // Edge::m_userp - New edge if we have seen this edge already + m_graphp->userClearVertices(); + m_graphp->userClearEdges(); + (void) vertexIterateAll(vertexp); + } + ~GraphAlgSubtrees() {} +}; + +//! Report the entire connected graph with a loop or loops +void V3Graph::subtreeLoops(V3EdgeFuncP edgeFuncp, V3GraphVertex* vertexp, + V3Graph* loopGraphp) { + GraphAlgSubtrees (this, loopGraphp, edgeFuncp, vertexp); +} + //###################################################################### //###################################################################### // Algorithms - make non cutable @@ -465,7 +517,9 @@ void V3Graph::order() { } } - // Sort list of vertices by rank, then fanout + // Sort list of vertices by rank, then fanout. Fanout is a bit of a + // misnomer. It is the sum of all the fanouts of nodes reached from a node + // *plus* the count of edges in to that node. sortVertices(); // Sort edges by rank then fanout of node they point to sortEdges(); diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 40ef9af83..db3f43f70 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -741,6 +741,7 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, char else if ( onoff (sw, "-profile-cfuncs", flag/*ref*/) ) { m_profileCFuncs = flag; } else if ( onoff (sw, "-psl", flag/*ref*/) ) { m_psl = flag; } else if ( onoff (sw, "-public", flag/*ref*/) ) { m_public = flag; } + else if ( onoff (sw, "-report-unoptflat", flag/*ref*/) ) { m_reportUnoptflat = flag; } else if ( onoff (sw, "-savable", flag/*ref*/) ) { m_savable = flag; } else if ( !strcmp (sw, "-sc") ) { m_outFormatOk = true; m_systemC = true; m_systemPerl = false; } else if ( onoff (sw, "-skip-identical", flag/*ref*/) ) { m_skipIdentical = flag; } @@ -1213,6 +1214,7 @@ V3Options::V3Options() { m_traceDups = false; m_traceUnderscore = false; m_underlineZero = false; + m_reportUnoptflat = false; m_xInitialEdge = false; m_xmlOnly = false; diff --git a/src/V3Options.h b/src/V3Options.h index 5f6cd2939..55d0def8e 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -89,6 +89,7 @@ class V3Options { bool m_traceDups; // main switch: --trace-dups bool m_traceUnderscore;// main switch: --trace-underscore bool m_underlineZero;// main switch: --underline-zero; undocumented old Verilator 2 + bool m_reportUnoptflat; // main switch: --report-unoptflat bool m_xInitialEdge; // main switch: --x-initial-edge bool m_xmlOnly; // main switch: --xml-netlist @@ -222,6 +223,7 @@ class V3Options { bool lintOnly() const { return m_lintOnly; } bool ignc() const { return m_ignc; } bool inhibitSim() const { return m_inhibitSim; } + bool reportUnoptflat() const { return m_reportUnoptflat; } bool xInitialEdge() const { return m_xInitialEdge; } bool xmlOnly() const { return m_xmlOnly; } diff --git a/src/V3Order.cpp b/src/V3Order.cpp index e3f2456ad..e204c9f39 100644 --- a/src/V3Order.cpp +++ b/src/V3Order.cpp @@ -232,6 +232,26 @@ public: ~OrderUser() {} }; + +//###################################################################### +// Comparator classes + +//! Comparator for width of associated variable +struct OrderVarWidthCmp { + bool operator() (OrderVarStdVertex* vsv1p, OrderVarStdVertex* vsv2p) { + return vsv1p->varScp()->varp()->width() + > vsv2p->varScp()->varp()->width(); + } +}; + +//! Comparator for fanout of vertex +struct OrderVarFanoutCmp { + bool operator() (OrderVarStdVertex* vsv1p, OrderVarStdVertex* vsv2p) { + return vsv1p->fanout() > vsv2p->fanout(); + } +}; + + //###################################################################### // Order class functions @@ -285,6 +305,7 @@ private: protected: friend class OrderMoveDomScope; V3List m_pomReadyDomScope; // List of ready domain/scope pairs, by loopId + vector m_unoptflatVars; // Vector of variables in UNOPTFLAT loop private: // STATS @@ -418,10 +439,99 @@ private: if (tempWeight) edgep->weight(1); // Else the below loop detect can't see the loop m_graph.reportLoops(&OrderEdge::followComboConnected, vertexp); // calls OrderGraph::loopsVertexCb if (tempWeight) edgep->weight(0); + if (v3Global.opt.reportUnoptflat()) { + // Report candidate variables for splitting + reportLoopVars(vertexp); + // Do a subgraph for the UNOPTFLAT loop + OrderGraph loopGraph; + m_graph.subtreeLoops(&OrderEdge::followComboConnected, + vertexp, &loopGraph); + loopGraph.dumpDotFilePrefixedAlways("unoptflat"); + } } } } + //! Find all variables in an UNOPTFLAT loop + //! + //! Ignore vars that are 1-bit wide and don't worry about generated + //! variables (PRE and POST vars, __Vdly__, __Vcellin__ and __VCellout). + //! What remains are candidates for splitting to break loops. + //! + //! node->user3 is used to mark if we have done a particular variable. + //! vertex->user is used to mark if we have seen this vertex before. + //! + //! @todo We could be cleverer in the future and consider just + //! the width that is generated/consumed. + void reportLoopVars(OrderVarVertex* vertexp) { + m_graph.userClearVertices(); + AstNode::user3ClearTree(); + m_unoptflatVars.clear(); + reportLoopVarsIterate (vertexp, vertexp->color()); + AstNode::user3ClearTree(); + m_graph.userClearVertices(); + // May be very large vector, so only report the "most important" + // elements. Up to 10 of the widest + cerr<varScp()->varp(); + cerr<fileline()<<" "<prettyName()<width()<<", fanout " + <fanout()<varScp()->varp(); + cerr<fileline()<<" "<prettyName() + <<", width "<width() + <<", fanout "<fanout()<user()) return; // Already done + vertexp->user(1); + if (OrderVarStdVertex* vsvertexp = dynamic_cast(vertexp)) { + // Only reporting on standard variable vertices + AstVar* varp = vsvertexp->varScp()->varp(); + if (!varp->user3()) { + string name = varp->prettyName(); + if ((varp->width() != 1) + && (name.find("__Vdly") == string::npos) + && (name.find("__Vcell") == string::npos)) { + // Variable to report on and not yet done + m_unoptflatVars.push_back(vsvertexp); + } + varp->user3Inc(); + } + } + // Iterate through all the to and from vertices of the same color + for (V3GraphEdge* edgep = vertexp->outBeginp(); edgep; + edgep = edgep->outNextp()) { + if (edgep->top()->color() == color) { + reportLoopVarsIterate(edgep->top(), color); + } + } + for (V3GraphEdge* edgep = vertexp->inBeginp(); edgep; + edgep = edgep->inNextp()) { + if (edgep->fromp()->color() == color) { + reportLoopVarsIterate(edgep->fromp(), color); + } + } + } // VISITORS virtual void visit(AstNetlist* nodep, AstNUser*) { { @@ -1600,7 +1710,10 @@ void OrderVisitor::process() { m_graph.acyclic(&V3GraphEdge::followAlwaysTrue); m_graph.dumpDotFilePrefixed("orderg_preasn_done", true); #else - // Break cycles + // Break cycles. Each strongly connected subgraph (including cutable + // edges) will have its own color, and corresponds to a loop in the + // original graph. However the new graph will be acyclic (the removed + // edges are actually still there, just with weight 0). UINFO(2," Acyclic & Order...\n"); m_graph.acyclic(&V3GraphEdge::followAlwaysTrue); m_graph.dumpDotFilePrefixed("orderg_acyc"); @@ -1611,6 +1724,9 @@ void OrderVisitor::process() { m_graph.order(); m_graph.dumpDotFilePrefixed("orderg_order"); + // This finds everything that can be traced from an input (which by + // definition are the source clocks). After this any vertex which was + // traced has isFromInput() true. UINFO(2," Process Clocks...\n"); processInputs(); // must be before processCircular diff --git a/src/V3OrderGraph.h b/src/V3OrderGraph.h index b0bf3e5e2..4fcf6dc26 100644 --- a/src/V3OrderGraph.h +++ b/src/V3OrderGraph.h @@ -123,6 +123,15 @@ public: virtual void loopsVertexCb(V3GraphVertex* vertexp); }; +//! Graph for UNOPTFLAT loops +class UnoptflatGraph : public OrderGraph { +public: + UnoptflatGraph() {} + virtual ~UnoptflatGraph() {} + // Methods + virtual void loopsVertexCb(V3GraphVertex* vertexp); +}; + //###################################################################### // Vertex types @@ -336,9 +345,9 @@ public: }; class OrderLoopEndVertex : public OrderLogicVertex { - // A end vertex points to the *same nodep* as the Begin, - // as we need it to be a logic vertex for moving, but don't need a permanent node. - // We won't add to the output graph though, so it shouldn't matter. + // A end vertex points to the *same nodep* as the Begin, as we need it to + // be a logic vertex for moving, but don't need a permanent node. We + // won't add to the output graph though, so it shouldn't matter. OrderLoopBeginVertex* m_beginVertexp; // Corresponding loop begin OrderLoopEndVertex(V3Graph* graphp, const OrderLoopEndVertex& old) : OrderLogicVertex(graphp, old), m_beginVertexp(old.m_beginVertexp) {} diff --git a/test_regress/t/t_unoptflat_simple.v b/test_regress/t/t_unoptflat_simple.v new file mode 100644 index 000000000..f3e5be00d --- /dev/null +++ b/test_regress/t/t_unoptflat_simple.v @@ -0,0 +1,32 @@ +// DESCRIPTION: Verilator: Simple test of unoptflat +// +// Simple demonstration of an UNOPTFLAT combinatorial loop, using just 2 bits. +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2013 by Jeremy Bennett. + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + wire [1:0] x = { x[0], clk }; + + initial begin + x = 0; + end + + always @(posedge clk or negedge clk) begin + +`ifdef TEST_VERBOSE + $write("x = %x\n", x); +`endif + + if (x[1] != 0) begin + $write("*-* All Finished *-*\n"); + $finish; + end + end + +endmodule diff --git a/test_regress/t/t_unoptflat_simple_2.v b/test_regress/t/t_unoptflat_simple_2.v new file mode 100644 index 000000000..13000264c --- /dev/null +++ b/test_regress/t/t_unoptflat_simple_2.v @@ -0,0 +1,35 @@ +// DESCRIPTION: Verilator: Simple test of unoptflat +// +// Simple demonstration of an UNOPTFLAT combinatorial loop, using 3 bits. +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2013 by Jeremy Bennett. + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + wire [2:0] x; + + initial begin + x = 3'b000; + end + + assign x[1:0] = { x[0], clk }; + assign x[2:1] = { clk, x[1] }; + + always @(posedge clk or negedge clk) begin + +`ifdef TEST_VERBOSE + $write("x = %x\n", x); +`endif + + if (x[1] != 0) begin + $write("*-* All Finished *-*\n"); + $finish; + end + end + +endmodule // t diff --git a/test_regress/t/t_unoptflat_simple_2_bad.pl b/test_regress/t/t_unoptflat_simple_2_bad.pl new file mode 100755 index 000000000..77d7a8066 --- /dev/null +++ b/test_regress/t/t_unoptflat_simple_2_bad.pl @@ -0,0 +1,24 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 by Wilson Snyder. 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. + +top_filename("t/t_unoptflat_simple_2.v"); + +# Compile only +compile ( + verilator_flags2 => ["--report-unoptflat"], + fails => 1, + expect=> +'.*%Warning-UNOPTFLAT: Widest candidate vars to split: +%Warning-UNOPTFLAT: t/t_unoptflat_simple_2.v:\d+: v.x, width 3, fanout 12 +.*%Error: Exiting due to ', + ); + + +ok(1); +1; diff --git a/test_regress/t/t_unoptflat_simple_3.v b/test_regress/t/t_unoptflat_simple_3.v new file mode 100644 index 000000000..d8a469169 --- /dev/null +++ b/test_regress/t/t_unoptflat_simple_3.v @@ -0,0 +1,79 @@ +// DESCRIPTION: Verilator: Simple test of unoptflat +// +// Demonstration of an UNOPTFLAT combinatorial loop using 3 bits and looping +// through 2 sub-modules. +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2013 by Jeremy Bennett. + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + wire [2:0] x; + + initial begin + x = 3'b000; + end + + test1 test1i ( .clk (clk), + .xvecin (x[1:0]), + .xvecout (x[2:1])); + + test2 test2i ( .clk (clk), + .xvecin (x[2:1]), + .xvecout (x[1:0])); + + always @(posedge clk or negedge clk) begin + +`ifdef TEST_VERBOSE + $write("x = %x\n", x); +`endif + + if (x[1] != 0) begin + $write("*-* All Finished *-*\n"); + $finish; + end + end + +endmodule // t + + +module test1 + (/*AUTOARG*/ + // Inputs + clk, + xvecin, + // Outputs + xvecout + ); + + input clk; + input wire [1:0] xvecin; + + output wire [1:0] xvecout; + + assign xvecout = {xvecin[0], clk}; + +endmodule // test + + +module test2 + (/*AUTOARG*/ + // Inputs + clk, + xvecin, + // Outputs + xvecout + ); + + input clk; + input wire [1:0] xvecin; + + output wire [1:0] xvecout; + + assign xvecout = {clk, xvecin[1]}; + +endmodule // test diff --git a/test_regress/t/t_unoptflat_simple_3_bad.pl b/test_regress/t/t_unoptflat_simple_3_bad.pl new file mode 100755 index 000000000..0c2bf1c5c --- /dev/null +++ b/test_regress/t/t_unoptflat_simple_3_bad.pl @@ -0,0 +1,18 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 by Wilson Snyder. 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. + +top_filename("t/t_unoptflat_simple_3.v"); + +# Compile only +compile ( + fails => 1 + ); + +ok(1); +1; diff --git a/test_regress/t/t_unoptflat_simple_bad.pl b/test_regress/t/t_unoptflat_simple_bad.pl new file mode 100755 index 000000000..ab27f0d1b --- /dev/null +++ b/test_regress/t/t_unoptflat_simple_bad.pl @@ -0,0 +1,18 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 by Wilson Snyder. 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. + +top_filename("t/t_unoptflat_simple.v"); + +# Compile only +compile ( + fails => 1 + ); + +ok(1); +1;