From 4afcb421bddfba7fa1912c0f41bbdb67a6eb564e Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Fri, 31 Dec 2010 18:36:29 -0500 Subject: [PATCH] With --Wall, add BLKSEQ warning on blocking assignments in seq blocks. --- Changes | 2 ++ bin/verilator | 18 +++++++++++++-- src/V3Active.cpp | 33 +++++++++++++++++++--------- src/V3Error.h | 9 +++++--- test_regress/t/t_lint_blksync_bad.pl | 26 ++++++++++++++++++++++ test_regress/t/t_lint_blksync_bad.v | 28 +++++++++++++++++++++++ 6 files changed, 101 insertions(+), 15 deletions(-) create mode 100755 test_regress/t/t_lint_blksync_bad.pl create mode 100644 test_regress/t/t_lint_blksync_bad.v diff --git a/Changes b/Changes index 6d84ddb79..b5e1631a4 100644 --- a/Changes +++ b/Changes @@ -18,6 +18,8 @@ indicates the contributor was also the author of the fix; Thanks! ** Add -Wall, -Wwarn-style, -Wno-style to enable code style warnings that have been added to this release, and disabled by default: +*** With --Wall, add BLKSEQ warning on blocking assignments in seq blocks. + *** With --Wall, add DECLFILENAME warning on modules not matching filename. *** With --Wall, add DEFPARAM warning on deprecated defparam statements. diff --git a/bin/verilator b/bin/verilator index f4caea3ab..54dc1ccce 100755 --- a/bin/verilator +++ b/bin/verilator @@ -1248,7 +1248,7 @@ for debugging and selecting between multiple operating system builds. Specifies the directory containing the distribution kit. This is used to find the executable, Perl library, and include files. If not specified, it will come from a default optionally specified at configure time (before -Verilator was compiled). It should not be spefified if using a precompiled +Verilator was compiled). It should not be specified if using a pre-compiled Verilator RPM as the hardcoded value should be correct. =back @@ -1420,7 +1420,7 @@ Instead of DPI exporting, there's also Verilator public functions, which are slightly faster, but less compatible. -=head1 VERIFICATION PROCEDURAL INTERACE (VPI) +=head1 VERIFICATION PROCEDURAL INTERFACE (VPI) Verilator supports a very limited subset of the VPI. This subset allows inspection, examination, value change callbacks, and depositing of values @@ -2329,6 +2329,20 @@ inside a public task. Ignoring this warning may make Verilator simulations differ from other simulators. +=item BLKSEQ + +This indicates that a blocking assignment (=) is used in a sequential +block. Generally non-blocking/delayed assignments (<=) are used in +sequential blocks, to avoid the possibility of simulator races. It can be +reasonable to do this if the generated signal is used ONLY later in the +same block, however this style is generally discouraged as it is error +prone. + + always @ (posedge clk) foo = ... + +Disabled by default as this is a code style warning; it will simulate +correctly. + =item BLKLOOPINIT This indicates that the initialization of an array needs to use non-delayed diff --git a/src/V3Active.cpp b/src/V3Active.cpp index 03d9ddfd7..aa6658e07 100644 --- a/src/V3Active.cpp +++ b/src/V3Active.cpp @@ -160,16 +160,25 @@ public: class ActiveDlyVisitor : public ActiveBaseVisitor { private: + bool m_combo; // Combo logic // VISITORS virtual void visit(AstAssignDly* nodep, AstNUser*) { - // Convert to a non-delayed assignment - UINFO(5," ASSIGNDLY "<v3warn(COMBDLY,"Delayed assignments (<=) in non-clocked (non flop or latch) blocks should be non-delayed assignments (=)."); - AstNode* newp = new AstAssign (nodep->fileline(), - nodep->lhsp()->unlinkFrBack(), - nodep->rhsp()->unlinkFrBack()); - nodep->replaceWith(newp); - nodep->deleteTree(); nodep = NULL; + if (m_combo) { + // Convert to a non-delayed assignment + UINFO(5," ASSIGNDLY "<v3warn(COMBDLY,"Delayed assignments (<=) in non-clocked (non flop or latch) block; suggest blocking assignments (=)."); + AstNode* newp = new AstAssign (nodep->fileline(), + nodep->lhsp()->unlinkFrBack(), + nodep->rhsp()->unlinkFrBack()); + nodep->replaceWith(newp); + nodep->deleteTree(); nodep = NULL; + } + } + virtual void visit(AstAssign* nodep, AstNUser*) { + if (!m_combo) { + // Convert to a non-delayed assignment + nodep->v3warn(BLKSEQ,"Blocking assignments (=) in sequential (flop or latch) block; suggest delayed assignments (<=)."); + } } // Empty visitors, speed things up virtual void visit(AstNodeMath* nodep, AstNUser*) {} @@ -179,7 +188,8 @@ private: } public: // CONSTUCTORS - ActiveDlyVisitor(AstNode* nodep) { + ActiveDlyVisitor(AstNode* nodep, bool combo) { + m_combo = combo; nodep->accept(*this); } virtual ~ActiveDlyVisitor() {} @@ -317,7 +327,10 @@ private: // Warn and/or convert any delayed assignments if (combo && !sequent) { - ActiveDlyVisitor dlyvisitor (nodep); + ActiveDlyVisitor dlyvisitor (nodep, true); + } + else if (!combo && sequent) { + ActiveDlyVisitor dlyvisitor (nodep, false); } } virtual void visit(AstAlways* nodep, AstNUser*) { diff --git a/src/V3Error.h b/src/V3Error.h index b0992f1a6..78be421c4 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -54,6 +54,7 @@ public: EC_FIRST_WARN, // Just a code so the program knows where to start warnings // BLKANDNBLK, // Blocked and non-blocking assignments to same variable + BLKSEQ, // Blocking assignments in sequential block CASEINCOMPLETE, // Case statement has missing values CASEOVERLAP, // Case statements overlap CASEWITHX, // Case with X values @@ -103,13 +104,14 @@ public: "MULTITOP", "TASKNSVAR", "BLKLOOPINIT", // Warnings " EC_FIRST_WARN", - "BLKANDNBLK", + "BLKANDNBLK", "BLKSEQ", "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CDCRSTLOGIC", "CMPCONST", "COMBDLY", "DEFPARAM", "DECLFILENAME", "GENCLK", "IFDEPTH", "IMPERFECTSCH", "IMPLICIT", "IMPURE", "INCABSPATH", "LITENDIAN", "MODDUP", - "MULTIDRIVEN", "REDEFMACRO", + "MULTIDRIVEN", + "REDEFMACRO", "STMTDLY", "SYMRSVDWORD", "SYNCASYNCNET", "UNDRIVEN", "UNOPT", "UNOPTFLAT", "UNSIGNED", "UNUSED", "VARHIDDEN", "WIDTH", "WIDTHCONCAT", @@ -136,7 +138,8 @@ public: || m_e==UNSIGNED || m_e==WIDTH); } // Warnings that are style only - bool styleError() const { return ( m_e==DEFPARAM + bool styleError() const { return ( m_e==BLKSEQ + || m_e==DEFPARAM || m_e==DECLFILENAME || m_e==INCABSPATH || m_e==SYNCASYNCNET diff --git a/test_regress/t/t_lint_blksync_bad.pl b/test_regress/t/t_lint_blksync_bad.pl new file mode 100755 index 000000000..0ea11d328 --- /dev/null +++ b/test_regress/t/t_lint_blksync_bad.pl @@ -0,0 +1,26 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2008 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 ( + v_flags2 => ["--lint-only -Wwarn-BLKSEQ -Wwarn-COMBDLY"], + fails=>1, + verilator_make_gcc => 0, + make_top_shell => 0, + make_main => 0, + expect=> +'%Warning-BLKSEQ: t/t_lint_blksync_bad.v:\d+: Blocking assignments \(=\) in sequential \(flop or latch\) block; suggest delayed assignments \(<=\). +%Warning-BLKSEQ: Use .* to disable this message. +%Warning-COMBDLY: t/t_lint_blksync_bad.v:\d+: Delayed assignments \(<=\) in non-clocked \(non flop or latch\) block; suggest blocking assignments \(=\). +%Warning-COMBDLY: \*\*\* See the manual before disabling this, +%Warning-COMBDLY: else you may end up with different sim results. +%Error: Exiting due to.*', + ) if $Self->{v3}; + +ok(1); +1; diff --git a/test_regress/t/t_lint_blksync_bad.v b/test_regress/t/t_lint_blksync_bad.v new file mode 100644 index 000000000..d5618f9ce --- /dev/null +++ b/test_regress/t/t_lint_blksync_bad.v @@ -0,0 +1,28 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2010 by Wilson Snyder. + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + reg sync_blk; + reg sync_nblk; + reg combo_blk; + reg combo_nblk; + + always @(posedge clk) begin + sync_blk = 1'b1; + sync_nblk <= 1'b1; + end + + always @* begin + combo_blk = 1'b1; + combo_nblk <= 1'b1; + end + +endmodule +