Add --report-unoptflat, bug611.

Signed-off-by: Wilson Snyder <wsnyder@wsnyder.org>

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.

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<dot> 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<Arguments>
+section for more details.
+
 Ignoring this warning will only slow simulations, it will simulate
 correctly.
 

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:

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"; }

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();

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;
 

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; }
 

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<OrderMoveDomScope*>  m_pomReadyDomScope;	// List of ready domain/scope pairs, by loopId
+    vector<OrderVarStdVertex*>	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<<V3Error::msgPrefix()
+	    <<"     Widest candidate vars to split:"<<endl;
+	sort (m_unoptflatVars.begin(), m_unoptflatVars.end(), OrderVarWidthCmp());
+	int lim = m_unoptflatVars.size() < 10 ? m_unoptflatVars.size() : 10;
+	for (int i = 0; i < lim; i++) {
+	    OrderVarStdVertex* vsvertexp = m_unoptflatVars[i];
+	    AstVar* varp = vsvertexp->varScp()->varp();
+	    cerr<<V3Error::msgPrefix()<<"          "
+		<<varp->fileline()<<" "<<varp->prettyName()<<dec
+		<<", width "<<varp->width()<<", fanout "
+		<<vsvertexp->fanout()<<endl;
+	}
+	// Up to 10 of the most fanned out
+	cerr<<V3Error::msgPrefix()
+	    <<"     Most fanned out candidate vars to split:"<<endl;
+	sort (m_unoptflatVars.begin(), m_unoptflatVars.end(),
+	      OrderVarFanoutCmp());
+	lim = m_unoptflatVars.size() < 10 ? m_unoptflatVars.size() : 10;
+	for (int i = 0; i < lim; i++) {
+	    OrderVarStdVertex* vsvertexp = m_unoptflatVars[i];
+	    AstVar* varp = vsvertexp->varScp()->varp();
+	    cerr<<V3Error::msgPrefix()<<"          "
+		<<varp->fileline()<<" "<<varp->prettyName()
+		<<", width "<<dec<<varp->width()
+		<<", fanout "<<vsvertexp->fanout()<<endl;
+	}
+	m_unoptflatVars.clear();
+    }
+
+    void reportLoopVarsIterate(V3GraphVertex* vertexp, uint32_t color) {
+	if (vertexp->user()) return; // Already done
+	vertexp->user(1);
+	if (OrderVarStdVertex* vsvertexp = dynamic_cast<OrderVarStdVertex*>(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
 

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) {}

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

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

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;

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

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;

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;