Fix mis-optimizing gate assignments in unopt blocks, bug881.

Changes

@@ -23,6 +23,8 @@ indicates the contributor was also the author of the fix; Thanks!
 
 ****  Fix slice connections of arrays to ports, bug880. [Varun Koyyalagunta]
 
+****  Fix mis-optimizing gate assignments in unopt blocks, bug881. [Mike Thyer]
+
 ****  Fix clang compile warnings.
 
 

src/V3Gate.cpp

@@ -34,6 +34,7 @@
 #include <iomanip>
 #include <vector>
 #include <list>
+#include <map>
 
 #include "V3Global.h"
 #include "V3Gate.h"
@@ -346,6 +347,7 @@ private:
     }
 
     void optimizeSignals(bool allowMultiIn);
+    bool elimLogicOkOutputs(GateLogicVertex* consumeVertexp, const GateOkVisitor& okVisitor);
     void optimizeElimVar(AstVarScope* varscp, AstNode* logicp, AstNode* consumerp);
     void warnSignals();
     void consumedMark();
@@ -591,9 +593,16 @@ void GateVisitor::optimizeSignals(bool allowMultiIn) {
 			if (debug()>=5) logicp->dumpTree(cout,"\telimVar:  ");
 			if (debug()>=5) substp->dumpTree(cout,"\t  subst:  ");
 			++m_statSigs;
-			while (V3GraphEdge* edgep = vvertexp->outBeginp()) {
+			bool removedAllUsages = true;
+			for (V3GraphEdge* edgep = vvertexp->outBeginp();
+			     edgep; ) {
 			    GateLogicVertex* consumeVertexp = dynamic_cast<GateLogicVertex*>(edgep->top());
 			    AstNode* consumerp = consumeVertexp->nodep();
+			    if (!elimLogicOkOutputs(consumeVertexp, okVisitor/*ref*/)) {
+				// Cannot optimize this replacement
+				removedAllUsages = false;
+				edgep = edgep->outNextp();
+			    } else {
 				optimizeElimVar(vvertexp->varScp(), substp, consumerp);
 				// If the new replacement referred to a signal,
 				// Correct the graph to point to this new generating variable
@@ -610,7 +619,10 @@ void GateVisitor::optimizeSignals(bool allowMultiIn) {
 				// Remove the edge
 				edgep->unlinkDelete(); edgep=NULL;
 				++m_statRefs;
+				edgep = vvertexp->outBeginp();
 			    }
+			}
+			if (removedAllUsages) {
 			    // Remove input links
 			    while (V3GraphEdge* edgep = vvertexp->inBeginp()) {
 				edgep->unlinkDelete(); edgep=NULL;
@@ -630,6 +642,31 @@ void GateVisitor::optimizeSignals(bool allowMultiIn) {
 		}
 	    }
 	}
+    }
+}
+
+bool GateVisitor::elimLogicOkOutputs(GateLogicVertex* consumeVertexp, const GateOkVisitor& okVisitor) {
+    // Return true if can optimize
+    // Return false if the consuming logic has an output signal that the replacement logic has as an input
+    typedef set<AstVarScope*> VarScopeSet;
+    // Use map to find duplicates between two lists
+    VarScopeSet varscopes;
+    // Replacement logic usually has shorter input list, so faster to build list based on it
+    const GateVarRefList& rhsVarRefs = okVisitor.rhsVarRefs();
+    for (GateVarRefList::const_iterator it = rhsVarRefs.begin();
+	 it != rhsVarRefs.end(); ++it) {
+	AstVarScope* vscp = (*it)->varScopep();
+	if (varscopes.find(vscp) == varscopes.end()) varscopes.insert(vscp);
+    }
+    for (V3GraphEdge* edgep = consumeVertexp->outBeginp(); edgep; edgep = edgep->outNextp()) {
+	GateVarVertex* consVVertexp = dynamic_cast<GateVarVertex*>(edgep->top());
+	AstVarScope* vscp = consVVertexp->varScp();
+	if (varscopes.find(vscp) != varscopes.end()) {
+	    UINFO(9,"    Block-unopt, insertion generates input vscp "<<vscp<<endl);
+	    return false;
+	}
+    }
+    return true;
 }
 
 void GateVisitor::replaceAssigns() {

test_regress/t/t_assign_inline.pl

@@ -0,0 +1,19 @@
+#!/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.
+
+compile (
+    verilator_flags2 => ["-O0 -OG"],
+    );
+
+execute (
+    check_finished=>1,
+    );
+
+ok(1);
+1;

test_regress/t/t_assign_inline.v

@@ -0,0 +1,52 @@
+// DESCRIPTION: Verilator: Verilog Test module 
+//
+// This file ONLY is placed into the Public Domain, for any use,
+// without warranty, 2015 by Mike Thyer.
+
+module t (/*AUTOARG*/
+   // Inputs
+   clk
+   );
+   input clk;
+   
+   int cycle=0;
+      
+   // verilator lint_off UNOPTFLAT
+   reg  [7:0] a_r;
+   wire [7:0] a_w;
+   reg  [7:0] b_r;
+   reg  [7:0] c_d_r, c_q_r;
+   
+   assign a_w = a_r;
+    
+   always @(*) begin
+     a_r = 0;
+     b_r = a_w;  // Substituting the a_w assignment to get b_r = 0 is wrong, as a_r is not "complete"
+     a_r = c_q_r;
+     c_d_r = c_q_r;
+   end
+
+   // stimulus + checks
+   always @(posedge clk) begin
+     cycle <= cycle+1;
+     if (cycle==0) begin
+       c_q_r <= 8'b0;
+     end
+     else begin 
+       c_q_r <= c_d_r+1;
+`ifdef TEST_VERBOSE
+       $display("[%0t] a_r=%0d, b_r=%0d", $time, a_r, b_r); // a_r and b_r should always be the same
+`endif
+     end
+     if (cycle >= 10) begin
+       if (b_r==9) begin
+         $write("*-* All Finished *-*\n");
+         $finish;
+       end
+       else begin
+         $stop;
+       end
+     end
+   end
+
+endmodule

test_regress/t/t_order_wireloop.pl

@@ -9,8 +9,10 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di
 
 compile (
     fails=>$Self->{v3},
+    # Used to be %Error: t/t_order_wireloop.v:\d+: Wire inputs its own output, creating circular logic .wire x=x.
+    # However we no longer gate optimize this
 	 expect=>
-'%Error: t/t_order_wireloop.v:\d+: Wire inputs its own output, creating circular logic .wire x=x.
+'%Warning-UNOPT: t/t_order_wireloop.v:\d+: Signal unoptimizable: Feedback to public clock or circular logic: bar
 ',
 	 );
 

test_regress/t/t_order_wireloop.v

@@ -3,10 +3,13 @@
 // This file ONLY is placed into the Public Domain, for any use,
 // without warranty, 2005 by Wilson Snyder.
 
-module t (/*AUTOARG*/);
+module t (/*AUTOARG*/
+   // Outputs
+   bar
+   );
 
    wire  foo;
-   wire  bar;
+   output  bar;
 
    // Oh dear.
    assign  foo = bar;