From e57d00471845a88d92a551dbf94f3d9a5016acbf Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Tue, 16 Mar 2010 18:50:26 -0400 Subject: [PATCH] Fix clock-gates with non-AND complex logic, bug220. --- Changes | 2 + bin/verilator | 3 +- src/V3Order.cpp | 53 ++++++--- src/V3OrderGraph.h | 14 ++- test_regress/driver.pl | 2 +- test_regress/t/t_clk_latchgate.pl | 18 +++ test_regress/t/t_clk_latchgate.v | 178 ++++++++++++++++++++++++++++++ 7 files changed, 247 insertions(+), 23 deletions(-) create mode 100755 test_regress/t/t_clk_latchgate.pl create mode 100644 test_regress/t/t_clk_latchgate.v diff --git a/Changes b/Changes index c65e7a197..338a5c249 100644 --- a/Changes +++ b/Changes @@ -11,6 +11,8 @@ indicates the contributor was also the author of the fix; Thanks! **** Skip SystemC tests if not installed. [Iztok Jeras] +**** Fix clock-gates with non-AND complex logic, bug220. [Ashutosh Das] + **** Fix flushing VCD buffers on $stop. [Ashutosh Das] **** Fix Mac OS-X compile issues, bug217. [Joshua Wise, Trevor Williams] diff --git a/bin/verilator b/bin/verilator index fd76ac423..1dcaaf34d 100755 --- a/bin/verilator +++ b/bin/verilator @@ -1629,7 +1629,8 @@ Used after a signal declaration to indicate the signal is used to gate a clock, and the user takes responsibility for insuring there are no races related to it. (Typically by adding a latch, and running static timing analysis.) This will cause the clock gate to be ignored in the scheduling -algorithm, improving performance. +algorithm, improving performance. It's also a good idea to enable the +IMPERFECTSCH warning, to insure all clock enables are properly recognized. =item /*verilator coverage_block_off*/ diff --git a/src/V3Order.cpp b/src/V3Order.cpp index a2aca0097..5eb0c0fa4 100644 --- a/src/V3Order.cpp +++ b/src/V3Order.cpp @@ -941,28 +941,51 @@ void OrderVisitor::processBrokeLoop() { // Clock propagation void OrderVisitor::processInputs() { - m_graph.userClearVertices(); // Vertex::user() // true if added as begin/end + m_graph.userClearVertices(); // Vertex::user() // true if processed + // Start at input vertex, process from input-to-output order + m_inputsVxp->isFromInput(true); // By definition processInputsIterate(m_inputsVxp); } void OrderVisitor::processInputsIterate(OrderEitherVertex* vertexp) { // Propagate PrimaryIn through simple assignments if (vertexp->user()) return; // Already processed - //UINFO(9," InIt "<user(true); - if (OrderVarStdVertex* vvertexp = dynamic_cast(vertexp)) { - vvertexp->isFromInput(true); - } - for (V3GraphEdge* edgep = vertexp->outBeginp(); edgep; edgep=edgep->outNextp()) { - OrderEitherVertex* toVertexp = (OrderEitherVertex*)edgep->top(); - if (OrderVarStdVertex* vvertexp = dynamic_cast(toVertexp)) { - processInputsIterate(vvertexp); + if (0 && debug()>=9) { + UINFO(9," InIt "<(vertexp)) { + vvertexp->nodep()->dumpTree(cout,"- TT: "); } - if (OrderLogicVertex* vvertexp = dynamic_cast(toVertexp)) { - if (AstNodeAssign* nodep = vvertexp->nodep()->castNodeAssign()) { - if (nodep->lhsp()->castVarRef() - && nodep->rhsp()->castVarRef()) { - UINFO(9," Input reassignment: "<user(true); // Processing + // First handle all inputs to this vertex, in most cases they'll be already processed earlier + // Also, determine if this vertex is an input + int inonly = 1; // 0=no, 1=maybe, 2=yes until a no + for (V3GraphEdge* edgep = vertexp->inBeginp(); edgep; edgep=edgep->inNextp()) { + OrderEitherVertex* frVertexp = (OrderEitherVertex*)edgep->fromp(); + processInputsIterate(frVertexp); + if (frVertexp->isFromInput()) { + if (inonly==1) inonly = 2; + } else if (dynamic_cast(frVertexp)) { + // Ignore post assignments, just for ordering + } else { + //UINFO(9," InItStopDueTo "<isFromInput(true); + } + // If we're still an input, process all targets of this vertex + if (vertexp->isFromInput()) { + for (V3GraphEdge* edgep = vertexp->outBeginp(); edgep; edgep=edgep->outNextp()) { + OrderEitherVertex* toVertexp = (OrderEitherVertex*)edgep->top(); + if (OrderVarStdVertex* vvertexp = dynamic_cast(toVertexp)) { + processInputsIterate(vvertexp); + } + if (OrderLogicVertex* vvertexp = dynamic_cast(toVertexp)) { + if (vvertexp->nodep()->castNodeAssign()) { processInputsIterate(vvertexp); } } diff --git a/src/V3OrderGraph.h b/src/V3OrderGraph.h index bb04cc81b..4e7f879e9 100644 --- a/src/V3OrderGraph.h +++ b/src/V3OrderGraph.h @@ -132,10 +132,11 @@ class OrderEitherVertex : public V3GraphVertex { AstScope* m_scopep; // Scope the vertex is in AstSenTree* m_domainp; // Clock domain (NULL = to be computed as we iterate) OrderLoopId m_inLoop; // Loop number vertex is in + bool m_isFromInput; // From input, or derrived therefrom (conservatively false) public: OrderEitherVertex(V3Graph* graphp, AstScope* scopep, AstSenTree* domainp) : V3GraphVertex(graphp), m_scopep(scopep), m_domainp(domainp) - , m_inLoop(LOOPID_UNKNOWN) { + , m_inLoop(LOOPID_UNKNOWN), m_isFromInput(false) { } virtual ~OrderEitherVertex() {} // Methods @@ -148,12 +149,16 @@ public: AstSenTree* domainp() const { return m_domainp; } OrderLoopId inLoop() const { return m_inLoop; } void inLoop(OrderLoopId inloop) { m_inLoop = inloop; } + void isFromInput(bool flag) { m_isFromInput=flag; } + bool isFromInput() const { return m_isFromInput; } }; class OrderInputsVertex : public OrderEitherVertex { public: OrderInputsVertex(V3Graph* graphp, AstSenTree* domainp) - : OrderEitherVertex(graphp, NULL, domainp) {} + : OrderEitherVertex(graphp, NULL, domainp) { + isFromInput(true); // By definition + } virtual ~OrderInputsVertex() {} virtual OrderVEdgeType type() const { return OrderVEdgeType::VERTEX_INPUTS; } virtual string name() const { return "*INPUTS*"; } @@ -195,11 +200,10 @@ class OrderVarVertex : public OrderEitherVertex { AstVarScope* m_varScp; OrderVarVertex* m_pilNewVertexp; // for processInsLoopNewVar bool m_isClock; // Used as clock - bool m_isFromInput; // From input, or derrived therefrom (conservatively false) public: OrderVarVertex(V3Graph* graphp, AstScope* scopep, AstVarScope* varScp) : OrderEitherVertex(graphp, scopep, NULL), m_varScp(varScp) - , m_pilNewVertexp(NULL), m_isClock(false), m_isFromInput(false) + , m_pilNewVertexp(NULL), m_isClock(false) {} virtual ~OrderVarVertex() {} virtual OrderVarVertex* clone (V3Graph* graphp) const = 0; @@ -208,8 +212,6 @@ public: AstVarScope* varScp() const { return m_varScp; } void isClock(bool flag) { m_isClock=flag; } bool isClock() const { return m_isClock; } - void isFromInput(bool flag) { m_isFromInput=flag; } - bool isFromInput() const { return m_isFromInput; } OrderVarVertex* pilNewVertexp() const { return m_pilNewVertexp; } void pilNewVertexp (OrderVarVertex* vertexp) { m_pilNewVertexp = vertexp; } }; diff --git a/test_regress/driver.pl b/test_regress/driver.pl index f03bce3eb..08783dbed 100755 --- a/test_regress/driver.pl +++ b/test_regress/driver.pl @@ -183,7 +183,7 @@ sub parameter { elsif ($param =~ /\.pl/) { push @opt_tests, $param; } - elsif ($param =~ /^--debugi/) { + elsif ($param =~ /^-?-debugi/) { push @Opt_Driver_Verilator_Flags, $param; $_Parameter_Next_Level = $param; } diff --git a/test_regress/t/t_clk_latchgate.pl b/test_regress/t/t_clk_latchgate.pl new file mode 100755 index 000000000..7058e622f --- /dev/null +++ b/test_regress/t/t_clk_latchgate.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. + +compile ( + ); + +execute ( + check_finished=>1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_clk_latchgate.v b/test_regress/t/t_clk_latchgate.v new file mode 100644 index 000000000..1e3503ff5 --- /dev/null +++ b/test_regress/t/t_clk_latchgate.v @@ -0,0 +1,178 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2010 by Wilson Snyder. +// +// -------------------------------------------------------- +// Bug Description: +// +// Issue: The gated clock gclk_vld[0] toggles but dvld[0] +// input to the flop does not propagate to the output +// signal entry_vld[0] correctly. The value that propagates +// is the new value of dvld[0] not the one just before the +// posedge of gclk_vld[0]. +// -------------------------------------------------------- + +// Define to see the bug with test failing with gated clock 'gclk_vld' +// Comment out the define to see the test passing with ungated clock 'clk' +`define GATED_CLK_TESTCASE 1 + +// A side effect of the problem is this warning, disabled by default +//verilator lint_on IMPERFECTSCH + +// Test Bench +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + integer cyc=0; + reg [63:0] crc; + + // Take CRC data and apply to testblock inputs + wire [7:0] dvld = crc[7:0]; + wire [7:0] ff_en_e1 = crc[15:8]; + + /*AUTOWIRE*/ + // Beginning of automatic wires (for undeclared instantiated-module outputs) + wire [7:0] entry_vld; // From test of Test.v + wire [7:0] ff_en_vld; // From test of Test.v + // End of automatics + + Test test (/*AUTOINST*/ + // Outputs + .ff_en_vld (ff_en_vld[7:0]), + .entry_vld (entry_vld[7:0]), + // Inputs + .clk (clk), + .dvld (dvld[7:0]), + .ff_en_e1 (ff_en_e1[7:0])); + + reg err_code; + reg ffq_clk_active; + reg [7:0] prv_dvld; + + initial begin + err_code = 0; + ffq_clk_active = 0; + end + always @ (posedge clk) begin + prv_dvld = test.dvld; + end + always @ (negedge test.ff_entry_dvld_0.clk) begin + ffq_clk_active = 1; + if (test.entry_vld[0] !== prv_dvld[0]) err_code = 1; + end + + // Test loop + always @ (posedge clk) begin +`ifdef TEST_VERBOSE + $write("[%0t] cyc==%0d crc=%x ",$time, cyc, crc); + $display(" en=%b fen=%b d=%b ev=%b", + test.flop_en_vld[0], test.ff_en_vld[0], + test.dvld[0], test.entry_vld[0]); +`endif + cyc <= cyc + 1; + crc <= {crc[62:0], crc[63]^crc[2]^crc[0]}; + if (cyc<3) begin + crc <= 64'h5aef0c8d_d70a4497; + end + else if (cyc==99) begin + $write("[%0t] cyc==%0d crc=%x\n",$time, cyc, crc); + if (ffq_clk_active == 0) begin + $display ("----"); + $display ("%%Error: TESTCASE FAILED with no Clock arriving at FFQs"); + $display ("----"); + $stop; + end + else if (err_code) begin + $display ("----"); + $display ("%%Error: TESTCASE FAILED with invalid propagation of 'd' to 'q' of FFQs"); + $display ("----"); + $stop; + end + else begin + $write("*-* All Finished *-*\n"); + $finish; + end + end + end + +endmodule + +module llq (clk, d, q); + parameter WIDTH = 32; + input clk; + input [WIDTH-1:0] d; + output [WIDTH-1:0] q; + + reg [WIDTH-1:0] qr; + + /* verilator lint_off COMBDLY */ + + always @(clk or d) + if (clk == 1'b0) + qr <= d; + + /* verilator lint_on COMBDLY */ + + assign q = qr; +endmodule + +module ffq (clk, d, q); + parameter WIDTH = 32; + input clk; + input [WIDTH-1:0] d; + output [WIDTH-1:0] q; + + reg [WIDTH-1:0] qr; + + always @(posedge clk) + qr <= d; + + assign q = qr; +endmodule + +// DUT module +module Test (/*AUTOARG*/ + // Outputs + ff_en_vld, entry_vld, + // Inputs + clk, dvld, ff_en_e1 + ); + input clk; + + input [7:0] dvld; + input [7:0] ff_en_e1; + + output [7:0] ff_en_vld; + output wire [7:0] entry_vld; + + wire [7:0] gclk_vld; + wire [7:0] ff_en_vld /*verilator clock_enable*/; + reg [7:0] flop_en_vld; + + always @(posedge clk) flop_en_vld <= ff_en_e1; + + // clock gating +`ifdef GATED_CLK_TESTCASE + assign gclk_vld = {8{clk}} & ff_en_vld; +`else + assign gclk_vld = {8{clk}}; +`endif + + // latch for avoiding glitch on the clock gating control + llq #(8) dp_ff_en_vld (.clk(clk), .d(flop_en_vld), .q(ff_en_vld)); + + // flops that use the gated clock signal + ffq #(1) ff_entry_dvld_0 (.clk(gclk_vld[0]), .d(dvld[0]), .q(entry_vld[0])); + ffq #(1) ff_entry_dvld_1 (.clk(gclk_vld[1]), .d(dvld[1]), .q(entry_vld[1])); + ffq #(1) ff_entry_dvld_2 (.clk(gclk_vld[2]), .d(dvld[2]), .q(entry_vld[2])); + ffq #(1) ff_entry_dvld_3 (.clk(gclk_vld[3]), .d(dvld[3]), .q(entry_vld[3])); + ffq #(1) ff_entry_dvld_4 (.clk(gclk_vld[4]), .d(dvld[4]), .q(entry_vld[4])); + ffq #(1) ff_entry_dvld_5 (.clk(gclk_vld[5]), .d(dvld[5]), .q(entry_vld[5])); + ffq #(1) ff_entry_dvld_6 (.clk(gclk_vld[6]), .d(dvld[6]), .q(entry_vld[6])); + ffq #(1) ff_entry_dvld_7 (.clk(gclk_vld[7]), .d(dvld[7]), .q(entry_vld[7])); + +endmodule