Fix ordering of clock enables with delayed assigns, bug613.

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.851 devel
 
+***   Fix ordering of clock enables with delayed assigns, bug613. [Jeremy Bennett]
+
 
 * Verilator 3.850 2013-06-02
 

src/V3Order.cpp

@@ -697,6 +697,13 @@ private:
 			    OrderVarVertex* varVxp = newVarUserVertex(varscp, WV_STD);
 			    if (m_inPost) {
 				new OrderPostCutEdge(&m_graph, m_logicVxp, varVxp);
+				// Mark the vertex. Used to control marking
+				// internal clocks circular, which must only
+				// happen if they are generated by delayed
+				// assignment.
+				UINFO(5, "     Found delayed assignment (post) "
+				      << varVxp << endl);
+				varVxp->isDelayed(true);
 			    } else {
 				new OrderComboCutEdge(&m_graph, m_logicVxp, varVxp);
 			    }
@@ -1146,10 +1153,21 @@ void OrderVisitor::processCircular() {
     for (V3GraphVertex* itp = m_graph.verticesBeginp(); itp; itp=itp->verticesNextp()) {
 	if (OrderVarStdVertex* vvertexp = dynamic_cast<OrderVarStdVertex*>(itp)) {
 	    if (vvertexp->isClock() && !vvertexp->isFromInput()) {
-		// If a clock is generated internally, we need to do another loop
-		// through the entire evaluation.  This fixes races; see t_clk_dpulse test.
-		UINFO(5,"Circular Clock "<<vvertexp<<endl);
-		nodeMarkCircular(vvertexp, NULL);
+		// If a clock is generated internally, we need to do another
+		// loop through the entire evaluation.  This fixes races; see
+		// t_clk_dpulse test.
+		//
+		// This all seems to hinge on how the clock is generated. If
+		// it is generated by delayed assignment, we need the loop. If
+		// it is combinatorial, we do not (and indeed it will break
+		// other tests such as t_gated_clk_1.
+		if (vvertexp->isDelayed()) {
+		    UINFO(5,"Circular Clock, delayed "<<vvertexp<<endl);
+		    nodeMarkCircular(vvertexp, NULL);
+		}
+		else {
+		    UINFO(5,"Circular Clock, not delayed "<<vvertexp<<endl);
+		}
 	    }
 	    // Also mark any cut edges
 	    for (V3GraphEdge* edgep = vvertexp->outBeginp(); edgep; edgep=edgep->outNextp()) {

src/V3OrderGraph.h

@@ -224,6 +224,7 @@ class OrderVarVertex : public OrderEitherVertex {
     AstVarScope* m_varScp;
     OrderVarVertex*	m_pilNewVertexp;	// for processInsLoopNewVar
     bool	 m_isClock;	// Used as clock
+    bool	 m_isDelayed;	// Set in a delayed assignment
 protected:
     OrderVarVertex(V3Graph* graphp, const OrderVarVertex& old)
 	: OrderEitherVertex(graphp, old)
@@ -231,7 +232,7 @@ protected:
 public:
     OrderVarVertex(V3Graph* graphp, AstScope* scopep, AstVarScope* varScp)
 	: OrderEitherVertex(graphp, scopep, NULL), m_varScp(varScp)
-	, m_pilNewVertexp(NULL), m_isClock(false) {}
+	, m_pilNewVertexp(NULL), m_isClock(false), m_isDelayed(false) {}
     virtual ~OrderVarVertex() {}
     virtual OrderVarVertex* clone (V3Graph* graphp) const = 0;
     virtual OrderVEdgeType type() const = 0;
@@ -239,6 +240,8 @@ public:
     AstVarScope* varScp() const { return m_varScp; }
     void isClock(bool flag) { m_isClock=flag; }
     bool isClock() const { return m_isClock; }
+    void isDelayed(bool flag) { m_isDelayed=flag; }
+    bool isDelayed() const { return m_isDelayed; }
     OrderVarVertex* pilNewVertexp() const { return m_pilNewVertexp; }
     void pilNewVertexp (OrderVarVertex* vertexp) { m_pilNewVertexp = vertexp; }
 };

test_regress/t/t_clk_condflop.pl

@@ -7,14 +7,11 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di
 # Lesser General Public License Version 3 or the Perl Artistic License
 # Version 2.0.
 
-my $fail = ($Self->{v3} && verilator_version() !~ /\(ord\)/);
-
 compile (
 	 );
 
 execute (
-	 check_finished => !$fail,
-	 fails => $fail,
+	 check_finished => 1
 	 );
 
 ok(1);

test_regress/t/t_clk_powerdn.pl

@@ -7,14 +7,11 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di
 # Lesser General Public License Version 3 or the Perl Artistic License
 # Version 2.0.
 
-my $fail = ($Self->{v3} && verilator_version() !~ /\(ord\)/);
-
 compile (
 	 );
 
 execute (
-	 check_finished => !$fail,
-	 fails => $fail,
+	 check_finished => 1
 	 );
 
 ok(1);

test_regress/t/t_gated_clk_1.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;

test_regress/t/t_gated_clk_1.v

@@ -0,0 +1,55 @@
+// DESCRIPTION: Verilator: Test of gated clock detection
+//
+// The code as shown generates a result by a delayed assignment from PC. The
+// creation of the result is from a clock gated from the clock that sets
+// PC. Howevever since they are essentially the same clock, the result should
+// be delayed by one cycle.
+//
+// Standard Verilator treats them as different clocks, so the result stays in
+// step with the PC. An event drive simulator always allows the clock to win.
+//
+// The problem is caused by the extra loop added by Verilator to the
+// evaluation of all internally generated clocks (effectively removed by
+// marking the clock enable).
+//
+// This test is added to facilitate experiments with solutions.
+//
+// This file ONLY is placed into the Public Domain, for any use,
+// without warranty, 2013 by Jeremy Bennett <jeremy.bennett@embecosm.com>.
+
+module t (/*AUTOARG*/
+   // Inputs
+   clk
+   );
+   input clk;
+
+   reg gated_clk_en = 1'b0 ;
+   reg [1:0] pc = 2'b0;
+   reg [1:0] res = 2'b0;
+
+   wire gated_clk = gated_clk_en & clk;
+
+   always @(posedge clk) begin
+      pc <= pc + 1;
+      gated_clk_en <= 1'b1;
+   end
+
+   always @(posedge gated_clk) begin
+      res <= pc;
+   end
+
+   always @(posedge clk) begin
+      if (pc == 2'b11) begin
+	 // Correct behaviour is that res should be lagging pc in the count
+	 // by one cycle
+	 if (res == 2'b10) begin
+	    $write("*-* All Finished *-*\n");
+	    $finish;
+	 end
+	 else begin
+	   $stop;
+	 end
+      end
+   end
+
+endmodule