diff --git a/Changes b/Changes index 8cac9986a..e3b2040b3 100644 --- a/Changes +++ b/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. diff --git a/src/V3Gate.cpp b/src/V3Gate.cpp index c2c49b5a6..25bef4992 100644 --- a/src/V3Gate.cpp +++ b/src/V3Gate.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #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,40 +593,51 @@ 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(edgep->top()); AstNode* consumerp = consumeVertexp->nodep(); - optimizeElimVar(vvertexp->varScp(), substp, consumerp); - // If the new replacement referred to a signal, - // Correct the graph to point to this new generating variable - const GateVarRefList& rhsVarRefs = okVisitor.rhsVarRefs(); - for (GateVarRefList::const_iterator it = rhsVarRefs.begin(); - it != rhsVarRefs.end(); ++it) { - AstVarScope* newvarscp = (*it)->varScopep(); - UINFO(9," Point-to-new vertex "<propagateAttrClocksFrom(vvertexp); + 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 + const GateVarRefList& rhsVarRefs = okVisitor.rhsVarRefs(); + for (GateVarRefList::const_iterator it = rhsVarRefs.begin(); + it != rhsVarRefs.end(); ++it) { + AstVarScope* newvarscp = (*it)->varScopep(); + UINFO(9," Point-to-new vertex "<propagateAttrClocksFrom(vvertexp); + } + // Remove the edge + edgep->unlinkDelete(); edgep=NULL; + ++m_statRefs; + edgep = vvertexp->outBeginp(); } - // Remove the edge - edgep->unlinkDelete(); edgep=NULL; - ++m_statRefs; } - // Remove input links - while (V3GraphEdge* edgep = vvertexp->inBeginp()) { - edgep->unlinkDelete(); edgep=NULL; + if (removedAllUsages) { + // Remove input links + while (V3GraphEdge* edgep = vvertexp->inBeginp()) { + edgep->unlinkDelete(); edgep=NULL; + } + // Clone tree so we remember it for tracing, and keep the pointer + // to the "ALWAYS" part of the tree as part of this statement + // That way if a later signal optimization that retained a pointer to the always + // can optimize it further + logicp->unlinkFrBack(); + vvertexp->varScp()->valuep(logicp); + logicp = NULL; + // Mark the vertex so we don't mark it as being unconsumed in the next step + vvertexp->user(true); + logicVertexp->user(true); } - // Clone tree so we remember it for tracing, and keep the pointer - // to the "ALWAYS" part of the tree as part of this statement - // That way if a later signal optimization that retained a pointer to the always - // can optimize it further - logicp->unlinkFrBack(); - vvertexp->varScp()->valuep(logicp); - logicp = NULL; - // Mark the vertex so we don't mark it as being unconsumed in the next step - vvertexp->user(true); - logicVertexp->user(true); } } } @@ -632,6 +645,30 @@ 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 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(edgep->top()); + AstVarScope* vscp = consVVertexp->varScp(); + if (varscopes.find(vscp) != varscopes.end()) { + UINFO(9," Block-unopt, insertion generates input vscp "<verticesNextp()) { if (GateVarVertex* vvertexp = dynamic_cast(itp)) { diff --git a/test_regress/t/t_assign_inline.pl b/test_regress/t/t_assign_inline.pl new file mode 100755 index 000000000..a433bb067 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_assign_inline.v b/test_regress/t/t_assign_inline.v new file mode 100644 index 000000000..cc1f25c5a --- /dev/null +++ b/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 diff --git a/test_regress/t/t_order_wireloop.pl b/test_regress/t/t_order_wireloop.pl index 285a33031..7a3675a5c 100755 --- a/test_regress/t/t_order_wireloop.pl +++ b/test_regress/t/t_order_wireloop.pl @@ -8,9 +8,11 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di # Version 2.0. compile ( - fails=>$Self->{v3}, + 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 ', ); diff --git a/test_regress/t/t_order_wireloop.v b/test_regress/t/t_order_wireloop.v index 1f4363ce3..b1156cf82 100644 --- a/test_regress/t/t_order_wireloop.v +++ b/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;