From 49c90ecbce45bd61df00bdfb5837823bfa25f9e3 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 29 Apr 2022 16:32:02 +0100 Subject: [PATCH] Issue consistent INITIALDLY/COMBDLY/BLKSEQ warnings Some cases of warnings about the use of blocking and non-blocking assignments in combinational vs sequential processes were suppressed in a way that is inconsistent with the *actual* current execution model of Verilator. Turning these back on to, well, warn the user that these might cause unexpected results. V5 will clean these up, but until then err on the side of caution. Fixes #864. --- Changes | 1 + src/V3Active.cpp | 105 +++++++++++------------- test_regress/t/t_delay_stmtdly_bad.out | 4 +- test_regress/t/t_initial_dlyass_bad.out | 8 +- test_regress/t/t_lint_blksync_bad.out | 12 ++- test_regress/t/t_lint_latch_1.out | 9 ++ test_regress/t/t_lint_latch_1.pl | 2 + test_regress/t/t_lint_latch_5.out | 13 +++ test_regress/t/t_lint_latch_5.pl | 2 + test_regress/t/t_lint_latch_bad.out | 16 ++-- test_regress/t/t_lint_latch_bad_2.out | 10 ++- test_regress/t/t_lint_latch_bad_3.out | 32 ++++++-- test_regress/t/t_lint_nolatch_bad.out | 10 ++- 13 files changed, 139 insertions(+), 85 deletions(-) create mode 100644 test_regress/t/t_lint_latch_1.out create mode 100644 test_regress/t/t_lint_latch_5.out diff --git a/Changes b/Changes index acac7dee0..ad4f5463c 100644 --- a/Changes +++ b/Changes @@ -16,6 +16,7 @@ Verilator 4.221 devel * Split --prof-threads into --prof-exec and --prof-pgo (#3365). [Geza Lore, Shunyao CAD] * Deprecate 'vluint64_t' and similar types (#3255). * Raise error on assignment to const in initial blocks. [Geza Lore, Shunyao CAD] +* Issue INITIALDLY/COMBDLY/BLKSEQ warnings consistent with Verilator execution. [Geza Lore, Shunyao CAD] * Fix MSVC localtime_s (#3124). * Fix Bison 3.8.2 error (#3366). [elike-ypq] * Fix rare bug in -Oz (V3Localize) (#3286). [Geza Lore, Shunyao CAD] diff --git a/src/V3Active.cpp b/src/V3Active.cpp index a0ed963ef..9f1758bbb 100644 --- a/src/V3Active.cpp +++ b/src/V3Active.cpp @@ -321,75 +321,70 @@ public: }; //###################################################################### -// Active AssignDly replacement functions +// Replace unsupported non-blocking assignments with blocking assignments class ActiveDlyVisitor final : public ActiveBaseVisitor { public: - enum CheckType : uint8_t { CT_SEQ, CT_COMBO, CT_INITIAL, CT_LATCH }; + enum CheckType : uint8_t { CT_SEQ, CT_COMB, CT_INITIAL }; private: - const CheckType m_check; // Combo logic or other - const AstNode* const m_alwaysp; // Always we're under - const AstNode* m_assignp = nullptr; // In assign + // MEMBERS + const CheckType m_check; // Process type we are checking + // VISITORS virtual void visit(AstAssignDly* nodep) override { - if (m_check != CT_SEQ) { - // Convert to a non-delayed assignment - UINFO(5, " ASSIGNDLY " << nodep << endl); - if (m_check == CT_INITIAL) { - nodep->v3warn(INITIALDLY, "Delayed assignments (<=) in initial or final block\n" - << nodep->warnMore() - << "... Suggest blocking assignments (=)"); - } else if (m_check == CT_LATCH) { - // Suppress. Shouldn't matter that the interior of the latch races - } else if (!(VN_IS(nodep->lhsp(), VarRef) - && VN_AS(nodep->lhsp(), VarRef)->varp()->isLatched())) { - nodep->v3warn(COMBDLY, "Delayed assignments (<=) in non-clocked" - " (non flop or latch) block\n" - << nodep->warnMore() - << "... Suggest blocking assignments (=)"); - // Conversely, we could also suggest latches use delayed assignments, as - // recommended by Cliff Cummings? - } - AstNode* const newp = new AstAssign(nodep->fileline(), nodep->lhsp()->unlinkFrBack(), - nodep->rhsp()->unlinkFrBack()); - nodep->replaceWith(newp); - VL_DO_DANGLING(nodep->deleteTree(), nodep); + // Non-blocking assignments are OK in sequential processes + if (m_check == CT_SEQ) return; + + // Issue appropriate warning + if (m_check == CT_INITIAL) { + nodep->v3warn(INITIALDLY, + "Non-blocking assignment '<=' in initial/final block\n" + << nodep->warnMore() + << "... This will be executed as a blocking assignment '='!"); + } else { + nodep->v3warn(COMBDLY, + "Non-blocking assignment '<=' in combinational logic process\n" + << nodep->warnMore() + << "... This will be executed as a blocking assignment '='!"); } + + // Convert to blocking assignment + nodep->replaceWith(new AstAssign{nodep->fileline(), // + nodep->lhsp()->unlinkFrBack(), // + nodep->rhsp()->unlinkFrBack()}); + VL_DO_DANGLING(nodep->deleteTree(), nodep); } + virtual void visit(AstAssign* nodep) override { - if (m_check == CT_SEQ) { - VL_RESTORER(m_assignp); - m_assignp = nodep; - iterateAndNextNull(nodep->lhsp()); - } - } - virtual void visit(AstVarRef* nodep) override { - const AstVar* const varp = nodep->varp(); - if (m_check == CT_SEQ && m_assignp && !varp->isUsedLoopIdx() // Ignore loop indices - && !varp->isTemp()) { - // Allow turning off warnings on the always, or the variable also - if (!m_alwaysp->fileline()->warnIsOff(V3ErrorCode::BLKSEQ) - && !m_assignp->fileline()->warnIsOff(V3ErrorCode::BLKSEQ) - && !varp->fileline()->warnIsOff(V3ErrorCode::BLKSEQ)) { - m_assignp->v3warn(BLKSEQ, - "Blocking assignments (=) in sequential (flop or latch) block\n" - << m_assignp->warnMore() - << "... Suggest delayed assignments (<=)"); - m_alwaysp->fileline()->modifyWarnOff( - V3ErrorCode::BLKSEQ, true); // Complain just once for the entire always - varp->fileline()->modifyWarnOff(V3ErrorCode::BLKSEQ, true); - } - } + // Blocking assignments are always OK in combinational (and initial/final) processes + if (m_check != CT_SEQ) return; + + const bool ignore = nodep->lhsp()->forall([&](const AstVarRef* refp) { + // Ignore reads (e.g.: index expressions) + if (refp->access().isReadOnly()) return true; + const AstVar* const varp = refp->varp(); + // Ignore ... + return varp->isUsedLoopIdx() // ... loop indices + || varp->isTemp() // ... temporaries + || varp->fileline()->warnIsOff(V3ErrorCode::BLKSEQ); // ... user said so + }); + + if (ignore) return; + + nodep->v3warn(BLKSEQ, + "Blocking assignment '=' in sequential logic process\n" + << nodep->warnMore() // + << "... Suggest using delayed assignment '<='"); } + //-------------------- virtual void visit(AstNode* nodep) override { iterateChildren(nodep); } public: // CONSTRUCTORS ActiveDlyVisitor(AstNode* nodep, CheckType check) - : m_check{check} - , m_alwaysp{nodep} { + : m_check{check} { iterate(nodep); } virtual ~ActiveDlyVisitor() override = default; @@ -535,12 +530,8 @@ private: // Warn and/or convert any delayed assignments if (combo && !sequent) { + ActiveDlyVisitor{nodep, ActiveDlyVisitor::CT_COMB}; const ActiveLatchCheckVisitor latchvisitor{nodep, kwd}; - if (kwd == VAlwaysKwd::ALWAYS_LATCH) { - ActiveDlyVisitor{nodep, ActiveDlyVisitor::CT_LATCH}; - } else { - ActiveDlyVisitor{nodep, ActiveDlyVisitor::CT_COMBO}; - } } else if (!combo && sequent) { ActiveDlyVisitor{nodep, ActiveDlyVisitor::CT_SEQ}; } diff --git a/test_regress/t/t_delay_stmtdly_bad.out b/test_regress/t/t_delay_stmtdly_bad.out index 8b8482f3e..87c088ddc 100644 --- a/test_regress/t/t_delay_stmtdly_bad.out +++ b/test_regress/t/t_delay_stmtdly_bad.out @@ -20,8 +20,8 @@ : ... In instance t 20 | dly_s_t dly_s; | ^~~~~ -%Warning-BLKSEQ: t/t_delay.v:37:20: Blocking assignments (=) in sequential (flop or latch) block - : ... Suggest delayed assignments (<=) +%Warning-BLKSEQ: t/t_delay.v:37:20: Blocking assignment '=' in sequential logic process + : ... Suggest using delayed assignment '<=' 37 | dly_s.dly = 55; | ^ %Error: Exiting due to diff --git a/test_regress/t/t_initial_dlyass_bad.out b/test_regress/t/t_initial_dlyass_bad.out index 686788c11..dfebc49eb 100644 --- a/test_regress/t/t_initial_dlyass_bad.out +++ b/test_regress/t/t_initial_dlyass_bad.out @@ -1,11 +1,11 @@ -%Warning-INITIALDLY: t/t_initial_dlyass.v:18:9: Delayed assignments (<=) in initial or final block - : ... Suggest blocking assignments (=) +%Warning-INITIALDLY: t/t_initial_dlyass.v:18:9: Non-blocking assignment '<=' in initial/final block + : ... This will be executed as a blocking assignment '='! 18 | a <= 22; | ^~ ... For warning description see https://verilator.org/warn/INITIALDLY?v=latest ... Use "/* verilator lint_off INITIALDLY */" and lint_on around source to disable this message. -%Warning-INITIALDLY: t/t_initial_dlyass.v:19:9: Delayed assignments (<=) in initial or final block - : ... Suggest blocking assignments (=) +%Warning-INITIALDLY: t/t_initial_dlyass.v:19:9: Non-blocking assignment '<=' in initial/final block + : ... This will be executed as a blocking assignment '='! 19 | b <= 33; | ^~ %Error: Exiting due to diff --git a/test_regress/t/t_lint_blksync_bad.out b/test_regress/t/t_lint_blksync_bad.out index 5491f6d0e..a9ca1c217 100644 --- a/test_regress/t/t_lint_blksync_bad.out +++ b/test_regress/t/t_lint_blksync_bad.out @@ -1,11 +1,15 @@ -%Warning-BLKSEQ: t/t_lint_blksync_bad.v:24:16: Blocking assignments (=) in sequential (flop or latch) block - : ... Suggest delayed assignments (<=) +%Warning-BLKSEQ: t/t_lint_blksync_bad.v:24:16: Blocking assignment '=' in sequential logic process + : ... Suggest using delayed assignment '<=' 24 | sync_blk = 1'b1; | ^ ... For warning description see https://verilator.org/warn/BLKSEQ?v=latest ... Use "/* verilator lint_off BLKSEQ */" and lint_on around source to disable this message. -%Warning-COMBDLY: t/t_lint_blksync_bad.v:31:18: Delayed assignments (<=) in non-clocked (non flop or latch) block - : ... Suggest blocking assignments (=) +%Warning-BLKSEQ: t/t_lint_blksync_bad.v:25:17: Blocking assignment '=' in sequential logic process + : ... Suggest using delayed assignment '<=' + 25 | sync_blk2 = 1'b1; + | ^ +%Warning-COMBDLY: t/t_lint_blksync_bad.v:31:18: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! 31 | combo_nblk <= 1'b1; | ^~ *** See https://verilator.org/warn/COMBDLY before disabling this, diff --git a/test_regress/t/t_lint_latch_1.out b/test_regress/t/t_lint_latch_1.out new file mode 100644 index 000000000..22f8aceb5 --- /dev/null +++ b/test_regress/t/t_lint_latch_1.out @@ -0,0 +1,9 @@ +%Warning-COMBDLY: t/t_lint_latch_1.v:14:10: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 14 | o <= b; + | ^~ + ... For warning description see https://verilator.org/warn/COMBDLY?v=latest + ... Use "/* verilator lint_off COMBDLY */" and lint_on around source to disable this message. + *** See https://verilator.org/warn/COMBDLY before disabling this, + else you may end up with different sim results. +%Error: Exiting due to diff --git a/test_regress/t/t_lint_latch_1.pl b/test_regress/t/t_lint_latch_1.pl index 629a44bbb..07964a1b5 100755 --- a/test_regress/t/t_lint_latch_1.pl +++ b/test_regress/t/t_lint_latch_1.pl @@ -11,6 +11,8 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(vlt => 1); lint( + fails => 1, + expect_filename => $Self->{golden_filename}, ); ok(1); diff --git a/test_regress/t/t_lint_latch_5.out b/test_regress/t/t_lint_latch_5.out new file mode 100644 index 000000000..30dab57fe --- /dev/null +++ b/test_regress/t/t_lint_latch_5.out @@ -0,0 +1,13 @@ +%Warning-COMBDLY: t/t_lint_latch_5.v:13:13: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 13 | z[0] <= a[0]; + | ^~ + ... For warning description see https://verilator.org/warn/COMBDLY?v=latest + ... Use "/* verilator lint_off COMBDLY */" and lint_on around source to disable this message. + *** See https://verilator.org/warn/COMBDLY before disabling this, + else you may end up with different sim results. +%Warning-COMBDLY: t/t_lint_latch_5.v:17:13: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 17 | z[1] <= a[1]; + | ^~ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_latch_5.pl b/test_regress/t/t_lint_latch_5.pl index 629a44bbb..07964a1b5 100755 --- a/test_regress/t/t_lint_latch_5.pl +++ b/test_regress/t/t_lint_latch_5.pl @@ -11,6 +11,8 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(vlt => 1); lint( + fails => 1, + expect_filename => $Self->{golden_filename}, ); ok(1); diff --git a/test_regress/t/t_lint_latch_bad.out b/test_regress/t/t_lint_latch_bad.out index e4c196c62..e24bb171c 100644 --- a/test_regress/t/t_lint_latch_bad.out +++ b/test_regress/t/t_lint_latch_bad.out @@ -1,12 +1,16 @@ +%Warning-COMBDLY: t/t_lint_latch_bad.v:18:10: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 18 | bl <= a; + | ^~ + ... For warning description see https://verilator.org/warn/COMBDLY?v=latest + ... Use "/* verilator lint_off COMBDLY */" and lint_on around source to disable this message. + *** See https://verilator.org/warn/COMBDLY before disabling this, + else you may end up with different sim results. %Warning-NOLATCH: t/t_lint_latch_bad.v:17:4: No latches detected in always_latch block 17 | always_latch begin | ^~~~~~~~~~~~ - ... For warning description see https://verilator.org/warn/NOLATCH?v=latest - ... Use "/* verilator lint_off NOLATCH */" and lint_on around source to disable this message. -%Warning-COMBDLY: t/t_lint_latch_bad.v:25:10: Delayed assignments (<=) in non-clocked (non flop or latch) block - : ... Suggest blocking assignments (=) +%Warning-COMBDLY: t/t_lint_latch_bad.v:25:10: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! 25 | bc <= a; | ^~ - *** See https://verilator.org/warn/COMBDLY before disabling this, - else you may end up with different sim results. %Error: Exiting due to diff --git a/test_regress/t/t_lint_latch_bad_2.out b/test_regress/t/t_lint_latch_bad_2.out index 129267f09..d5fb68aec 100644 --- a/test_regress/t/t_lint_latch_bad_2.out +++ b/test_regress/t/t_lint_latch_bad_2.out @@ -1,7 +1,13 @@ +%Warning-COMBDLY: t/t_lint_latch_bad_2.v:13:10: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 13 | o <= b; + | ^~ + ... For warning description see https://verilator.org/warn/COMBDLY?v=latest + ... Use "/* verilator lint_off COMBDLY */" and lint_on around source to disable this message. + *** See https://verilator.org/warn/COMBDLY before disabling this, + else you may end up with different sim results. %Warning-LATCH: t/t_lint_latch_bad_2.v:11:4: Latch inferred for signal 'o' (not all control paths of combinational always assign a value) : ... Suggest use of always_latch for intentional latches 11 | always @(a or b) | ^~~~~~ - ... For warning description see https://verilator.org/warn/LATCH?v=latest - ... Use "/* verilator lint_off LATCH */" and lint_on around source to disable this message. %Error: Exiting due to diff --git a/test_regress/t/t_lint_latch_bad_3.out b/test_regress/t/t_lint_latch_bad_3.out index 17b014783..b154019fa 100644 --- a/test_regress/t/t_lint_latch_bad_3.out +++ b/test_regress/t/t_lint_latch_bad_3.out @@ -1,13 +1,29 @@ +%Warning-COMBDLY: t/t_lint_latch_bad_3.v:25:8: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 25 | o5 <= 1'b0; + | ^~ + ... For warning description see https://verilator.org/warn/COMBDLY?v=latest + ... Use "/* verilator lint_off COMBDLY */" and lint_on around source to disable this message. + *** See https://verilator.org/warn/COMBDLY before disabling this, + else you may end up with different sim results. +%Warning-COMBDLY: t/t_lint_latch_bad_3.v:37:16: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 37 | o5 <= 1'b1; + | ^~ +%Warning-COMBDLY: t/t_lint_latch_bad_3.v:42:16: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 42 | o5 <= a; + | ^~ +%Warning-COMBDLY: t/t_lint_latch_bad_3.v:63:16: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 63 | o5 <= ~b; + | ^~ +%Warning-COMBDLY: t/t_lint_latch_bad_3.v:70:12: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 70 | o4 <= 1'b0; + | ^~ %Warning-LATCH: t/t_lint_latch_bad_3.v:18:1: Latch inferred for signal 'o5' (not all control paths of combinational always assign a value) : ... Suggest use of always_latch for intentional latches 18 | always @(reset or en or a or b) | ^~~~~~ - ... For warning description see https://verilator.org/warn/LATCH?v=latest - ... Use "/* verilator lint_off LATCH */" and lint_on around source to disable this message. -%Warning-COMBDLY: t/t_lint_latch_bad_3.v:70:12: Delayed assignments (<=) in non-clocked (non flop or latch) block - : ... Suggest blocking assignments (=) - 70 | o4 <= 1'b0; - | ^~ - *** See https://verilator.org/warn/COMBDLY before disabling this, - else you may end up with different sim results. %Error: Exiting due to diff --git a/test_regress/t/t_lint_nolatch_bad.out b/test_regress/t/t_lint_nolatch_bad.out index f4683645e..764e77aaa 100644 --- a/test_regress/t/t_lint_nolatch_bad.out +++ b/test_regress/t/t_lint_nolatch_bad.out @@ -1,6 +1,12 @@ +%Warning-COMBDLY: t/t_lint_nolatch_bad.v:13:10: Non-blocking assignment '<=' in combinational logic process + : ... This will be executed as a blocking assignment '='! + 13 | o <= b; + | ^~ + ... For warning description see https://verilator.org/warn/COMBDLY?v=latest + ... Use "/* verilator lint_off COMBDLY */" and lint_on around source to disable this message. + *** See https://verilator.org/warn/COMBDLY before disabling this, + else you may end up with different sim results. %Warning-NOLATCH: t/t_lint_nolatch_bad.v:11:4: No latches detected in always_latch block 11 | always_latch @(a or b) | ^~~~~~~~~~~~ - ... For warning description see https://verilator.org/warn/NOLATCH?v=latest - ... Use "/* verilator lint_off NOLATCH */" and lint_on around source to disable this message. %Error: Exiting due to