Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-20T03:36:26Z

Not sure I'm interpreting the question correctly, but...

Verilator doesn't presently identify self-identify latches (unless always_latch is used). If you'd like to contribute something to do this, and e.g. issue a LATCH warning and suppress the COMBDLY warning, I think that would be valuable. You would of course still need to disable the LATCH warning at the appropriate spots.

Original Redmine Comment
Author Name: Julien Margetts
Original Date: 2019-11-20T13:57:17Z

To ignore intentional standalone latches I think I need to update class ActiveDlyVisitor to supress the warning if the following conditions are met:

1. The always block containing the assignment is a 'combo' block
2. The sensitivity list for the combo block contains exactly 2 signals
3. There is only a single assignment in the always block (i.e this one)
4. (optional) The assignment is conditional on one of sensitised inputs
5. (optional) The assignment is to the other of the sensitied inputs

Exactly how to determine each of this things will be a bit of a learning exercise for me, (the first 2 look easy enough to figure out) but i'm keen to give it a go to understand the internals :)

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-20T23:48:46Z

Great, would be awesome to get this in.

The sensitivity list will at that point have some elements with combo on it. We should support arbitrary latches, it's fine to e.g. have reset or multiple assignments e.g.

always @ (...) 
    if (reset)
       a <= 0
       b <= 0
    else if (en)
       a <= c | d;
       b <= e | f;
    //otherwise recirc

First step would be to make a test file with a bunch of examples in it, run with debug and look at the *_delayed.tree file.

Logic would then build a V3Graph consisting of:

• A vertex for "start". Init "point" to this start vertex.

• A vertex for each control flow point. i.e. on a "AstNodeIf" you make a vertex for everything below the if. Add a edge from "point" to this new vertex. Everything below "if" then has point as that vertex. Repeat for else statements.

• A vertex for each output variable (AstVar with lvalue()=true). Add edge from point to this variable.

• A vertex for "output". Edge from each output variable to this vertex.

Now you can dump the graph (.dot) and see if it's what you expect for your example.

Then to report, traverse the graph making sure every path from input lands at every output.

Note V3Split has a lot of similar concepts, though more complicated. Search for V3Graph there's a lot of examples.

What really makes a latch is some output is not set in all control flow paths.

Then do the following:

if always && latch_detected => suppress COMBDLY, add a new LATCH warning suggesting fixing terms or use of always_latch.
if always_latch && ~latch_detected => error
if always_comb && latch_detected => error
if always_ff && latch_detected => error

Original Redmine Comment
Author Name: Julien Margetts
Original Date: 2019-12-05T13:48:07Z

Work in progress, but I have this basically working. I still need to generate more test cases and to also run a full regression.

I have implemented as a new pass called V3Latch, which may be a bit heavy weight (its only ~200 LOC) but it seemed the cleanest way to isolate the changes.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-05T17:35:29Z

Great, let me know when you have something for review. I'd also suggest running it on SweRV to see if it flags anything.

Leaving it V3Latch while you work on it is fine, we'll see what it looks like in the end as to if it should be wedged elsewhere.

Original Redmine Comment
Author Name: Peter Monsson
Original Date: 2019-12-15T18:05:10Z

Hi Julien,

I would like to help you out on this task. Is there anything that you would like to get off your plate?

Best Regards
Peter

Original Redmine Comment
Author Name: Julien Margetts
Original Date: 2019-12-17T15:56:20Z

This is pretty much complete I think, and seems to work well on the examples I have tried it on.
It also seems to survive the regression without adding any new failures, though I have not yet developed any directed testcases

PS: I'm sure this could be combined with the V3Active pass very easily if required, as most of the work is done in the new V3LatchGraph class

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-18T00:00:42Z

Wow, great stuff, I was expecting a LOT of questions before you had a fix!

It also seems to survive the regression without adding any new failures, though I have not yet developed any directed testcases

Had one minor issue, you need to add the .cpp file to src/Makefile_obj.in, I suspect you edited src/Makefile_obj but that's generated.

Please add some test cases that you believe test every non-internal-error branch in the code, see for example t_const_bad.*.

PS: I'm sure this could be combined with the V3Active pass very easily if required, as most of the work is done in the new V3LatchGraph class

Yes, please do that, as otherwise it is runtime-harmful.

 +++ b/src/V3Error.h
 @@ -119,6 +119,7 @@ public:
          WIDTHCONCAT,    // Unsized numbers/parameters in concatenations
 +        NOLATCH,        // No latch detected in always_latch block

Nit, please insert in sorted order.

Think you also want a LATCH warning (when any latch NOT in always_latch)?

 +++ b/src/V3Graph.h
 -    /// Any cutable edged become non-cutable
 +    /// Any cutable edges become non-cutable

I pushed this.

 +#define DISCONNECTED 0
 +#define CONNECTED    1

Need e.g. V3LATCH_ prefix on these (or renamed appropriately for whatever CPP file lands in), but it's probably cleaner to make an enum.

 +#ifdef V3LATCH_DEBUG
 +#define DBG_NAME(str) (str)
 +#else
 +#define DBG_NAME(str) "" /* Don't waste time naming vertices */
 +#endif

Likewise, need a prefix. Space indent "# define". Use // comment.

However, where this is used the only expensive one is "OUTPUT "+name, so perhaps just always do it and change that one not to have the "OUTPUT " prefix.

Another alternative is to hold the AstNode* in the vertex, which has the upside of making debug show the node information if needed.

 +class V3LatchGraph : public V3Graph {
 +protected:
 +    typedef std::vector<AstVarRef*> VarRefVec;
 +
 +    V3LatchGraphVertex *m_curr;
 +    VarRefVec           outputs;

m_ on all members. p suffix on pointers. Comment on members. "*" goes next to type. (Note clang-format will fix it for you, but it makes some things ugly). e.g.:

     V3LatchGraphVertex* m_vertexp;
     VarRefVec m_outputs;

 +        V3GraphEdge* edgep;

Move into the for initializer.

 +    // A new graph is created for each combinational always block encountered
 +    V3LatchGraph() { m_curr = new V3LatchGraphVertex(this, DBG_NAME("START")); }

Probably faster to have a clear() method and avoid re-new'ing.

 +    // Clear out userp field of referenced outputs on destruction
 +    // (occurs at the end of each combinational always block)
 +    virtual ~V3LatchGraph() {
 +        for (unsigned i=0;i<outputs.size();i++)
 +        {
 +            AstVarRef *vrp = outputs[i];
 +            vrp->varp()->user1p(NULL);
 +        }
 +        outputs.clear();

1. If ever using a user1p, you need a comment saying how used, and to declare your usage.

   // NODE STATE
   // Input:
   // AstVar::user1p // V3LatchGraphVertex* The vertex handling this node
   AstUser1InUse m_inuser1;

2. To clear all users just use AstNode::user1ClearTree(). It's effectively instantanious (no loop), so can call whereever.

   • void reset() {

   •    for (V3GraphVertex* vertexp = verticesBeginp();vertexp;vertexp=vertexp->verticesNextp()) {
     

   •        vertexp->color(DISCONNECTED);
     

If you use vertexp->user() instead of color you can then use m_graph.userClearVertices();

 +    // Add a new control path and connect it to its parent
 +    void add_path_vertex(V3LatchGraphVertex *parent, string name) {

const string& name

 +    // This code makes 2 assumptions, which are hopefully valid
 +    // 1.) user1p for each AstVar is NULL at the start of this pass

Yes, once you have AstUser1InUse, or call user1ClearTree()

 +    // 2.) It is safe to modify user1p during this pass, and not restore it to its previous value

Yes.

 +        #ifdef V3LATCH_DEBUG

if (debug() >= 9). Then run with "--debugi-V3Latch 9"

 +        removeRedundantEdges(&V3GraphEdge::followAlwaysTrue); // NOTE: Modifies vertex->user()
 +        dumpDotFile("V3Latch_"+nodep->fileline()->ascii()+".dot", true);

m_graph.dumpDotFilePrefixed("latch");

 +        for (unsigned i=0;i<outputs.size();i++)

Please use an iterator instead.

 +                nodep->v3warn(EC_INFO, "Latch inferred for signal '"<< vrp->name() <<

Use new LATCH warning.

 +            reset();

clear()?

 +class LatchVisitor : public AstNVisitor {
 +    V3LatchGraph*   m_gp;

Please rename m_graphp to match other places.

 +    virtual void visit(AstAlways* nodep) {

Please add m_enable = false here, as might be false-set from some earlier SenItem.

 +    virtual void visit(AstSenItem* nodep) {
 +        iterateChildren(nodep);
 +        if (nodep->edgeType() == VEdgeType::ET_ANYEDGE) {
 +            m_enable = true;
 +            m_gp = new V3LatchGraph();

This leaks (I think) for SenItem not under Always. Perhaps make once in constructor (don't need a pointer then), then m_graph.clear() on entry to AstAlways?

 +    explicit LatchVisitor(AstNetlist* nodep) {
 +        m_enable = false;

Need m_graphp = NULL, but by earlier comments perhaps not a pointer any longer.

Only just got back to this, sorry. Thanks for the review. I am tidying this up now.

A few comments/questions first:

If you use vertexp->user() instead of color you can then use m_graph.userClearVertices();

I did originally do this, but found removeRedundantEdges also uses the user field so currupted the graph in "debug" mode. color() seemed the next most logical field to use.

+ nodep->v3warn(EC_INFO, "Latch inferred for signal '"<< vrp->name() <<

Use new LATCH warning.

My reason for using EC_INFO here, instead of a new warning, was that I wanted an informational message which did not need to be supressed in order to successfully verilate the file in question. Because always_latch is SystemVerilog it cannot always be used. In my case, the file in question is third party IP, which cannot be modified, and must be compiled as pure Verilog. Is there a better way to issue a non-fatal warning?

Also, when merging into V3Active, shall I keep V3LatchGraph in it's own source file, or move the whole lot into V3Active.cpp?

Use LATCH, and mark it as a lintError() . If the user doesn't want it they can use -Wno-LATCH on the command line.

Please merge it into V3Active.cpp.

Getting there. Now 24 regression tests fail due to detected latches :) Need to review these and make sure they are expected.

Am I able to push my branch?

Let me review again and run on some private designs.

Sorry, I've lost where your branch was, can you ideally rebase to master, then do a github pull request with it?

It's just local at the moment, I was hoping to push to remote but I lack permissions, or I am using the wrong credentials.

I branched from master on Monday BTW.

I can't create a branch so posting another patch file:
issue_1609.txt

Compile issues:

../V3Active.cpp: In constructor 'LatchDetectGraphVertex::LatchDetectGraphVertex(V3Graph*, const string&, LatchDetectGraphVertex::VertexType)':
../V3Active.cpp:56:16: error: 'LatchDetectGraphVertex::m_name' will be initialized after [-Werror=reorder]
     string     m_name;       // Only used for .dot file generation
                ^~~~~~
../V3Active.cpp:55:16: error:   'LatchDetectGraphVertex::VertexType LatchDetectGraphVertex::m_type' [-Werror=reorder]
     VertexType m_type;
                ^~~~~~
../V3Active.cpp:66:5: error:   when initialized here [-Werror=reorder]
     LatchDetectGraphVertex(V3Graph* graphp, const string& name, VertexType type = VT_BLOCK)

Code review:

 --- a/src/V3Active.cpp
 +++ b/src/V3Active.cpp

+class LatchDetectGraph : public V3Graph {
+protected:
+    typedef std::vector<AstVarRef*> VarRefVec;
+
+    LatchDetectGraphVertex* m_curr;

Suggest rename m_curVertexp;

+    VarRefVec               m_outputs;

Need comments on all member variables please.

+    bool latch_check_internal(LatchDetectGraphVertex* vertexp) {

Please use latchCheckInternal style naming instead.

+    void begin() {
+        // Start a new graph

Start a new if/else tracking vertex.

+    LatchDetectGraphVertex* add_output_vertex(AstVarRef* nodep) {
+        LatchDetectGraphVertex *t = new LatchDetectGraphVertex(this, nodep->name(),

s/t/outVertexp/

+    // Connect an output assignment to its parent control block
+    void add_assignment(AstVarRef* nodep) {
+        LatchDetectGraphVertex* outp;

s/out/outVertexp/

+        LatchDetectGraphVertex* branchp;
+        branchp = m_graph.add_path_vertex(parentp, "BRANCH", true);

LatchDetectGraphVertex* branchp = m_graph.add_path_vertex(parentp, "BRANCH", true);

--- a/src/V3AstNodes.h
+++ b/src/V3AstNodes.h
@@ -1445,6 +1445,7 @@ AstVar::
+    bool        m_isLatch:1;    // Not assigned in all control paths of combo always

Given the var itself isn't a latch, I suggest renaming to m_isLatched,
likewise rename accessors. Please also update V3AstNodes.cpp's
AstVar::dump and V3EmitXml's Var dump routine to print this attribute.

  nodep->v3warn(LATCH, "Latch inferred for signal '"<< vrp->name() <<

vrp->prettyNameQ() and remove the quotes in your string.

+++ b/test_regress/t/t_lint_nolatch_bad.pl
+lint(
+    fails => 1
+    );

To all fail tests, please add expect_filename => $Self->{golden_filename},
so we check the warning message.

I get a lot of "make test_regress" failures. I think most all are false
failures so something needs tweaking.

Thanks. I'll get those done. I do not see the compilation errors, could this be down to GCC version? I am using a pretty old GCC (4.8.5) on RedHat7.

I was still expecting the failures as I am yet to review them all. I think they are all due to bombing out with latch warnings, so just need to check they are genuine. About 25 to go through in my run.

Would the prefered solution be to add -Wno-LATCH to the .pl files for tests which are known to create latches, or /* verilator lint_off LATCH */ to the .v files?

It might be the new gcc, but make sure you have VERILATOR_AUTHOR_SITE in your ~/bashrc/.cshrc so when you configure you get warnings.

There's probably only one or two tests that really have latches, for those please surround the lines that get warnings with the lint_off metacomments.

Looks like this has been stalled for a while, any interest on getting it dusted off and merged in?

Yes, very little spare bandwidth lately, but I will try ...

So having another look at this, and it is flagging latches here in regression (one of many examples):

%Warning-LATCH: t/t_altera_lpm.v:2714:5: Latch inferred for signal 'lpm_add_sub.tmp_cout' (not all control paths of combinational always assign a value)
                                       : ... Suggest use of always_latch for intentional latches
 2714 |     always @(cin or dataa or datab or i_add_sub)
      |     ^~~~~~

Looking at the code, this is a genuine latch detection since tmp_cout is not assigned in all control paths. Would you agree?

I think that this warning is appropriate. The module here is modeling code and not meant to be RTL as I understand it. (Totally looking forward to see this Linting rule in Verilator by the way).

@PeterMonsson, Thanks for the feedback. I am working through the regression failures. Many of them are similar to the above, and just require // verilator lint_off LATCH, which I am adding where appropriate.

I have other genuine false positives which I need to look into. Many of them are due to what appear to be internally generated signals (names starting __V) which my code is not distinguishing from genuine design signals. Is there a simple AstVar attribute I should query to filter these out? I could just ignore anything with the __V prefix, but this would be a bit naff.

I think I have answered my own question. AstVar::isSignal() seems to be what I want

Only 2 types of false positive left in test_regress now, and one of them is this:

   always @* begin
     if      (i==2'b00) o = 2'b11;
     else if (i==2'b01) o = 2'b10;
     else if (i==2'b10) o = 2'b01;
     else if (i==2'b11) o = 2'b00;
   //else << implicit else here need not assign o since it is unreachable
   end

Is there an easy way (during the V3Active pass) to tell that the implicit else node above is an un-reachable control path?

This isn't unreachable, it should give a LATCH warning as it is a latch if i=='x. The code should be changed to have either an else instead of elseif at the end, or to have "else o = 'x" at the end.

Ok, it's not unreachable, but for synthesis it effectively is. I'm not sure we should emit the warning here. Synthesis (DC) does not generate a latch here, or with the equivalent (fully defined) case statement:

   always @*
     case (i)
     2'b00: o = 2'b11;
     2'b01: o = 2'b10;
     2'b10: o = 2'b01;
     2'b11: o = 2'b00;
     endcase

Verilator will currently warn with the if/else variant, but not the case variant, which doesn't feel like the right thing to do?

Yes, we should have if/case operate the same and it's probably more intuitive to follow the synthesis rules.

It is going to be problematic eliminating the false positives in the remaining 2 failing test cases:

t/t_case_nest
t/t_case_wild

Consider the following:

always @* begin
   casez (in)
   10'b0000000000 : out = 4'h0;
   10'b?????????1 : out = 4'h0;
   10'b????????10 : out = 4'h1;
   10'b???????100 : out = 4'h2;
   10'b??????1000 : out = 4'h3;
   10'b?????10000 : out = 4'h4;
   10'b????100000 : out = 4'h5;
   10'b???1000000 : out = 4'h6;
   10'b??10000000 : out = 4'h7;
   10'b?100000000 : out = 4'h8;
   10'b1000000000 : out = 4'h9;
   endcase
end

Even though this covers all possible (2-state) combinations of 'in' and hence always assigns 'out', Verilator converts it to a series of if/else nodes with a dangling implicit else with no asignment. Latch detection just searches for control paths without an assignment, and in this case finds one, even though no value of 'in' could ever get to it.

To illustrate, here is the output C++

    if (((((((((0U == (IData)(vlTOPp->in)) | (1U == 
                                              (1U & (IData)(vlTOPp->in)))) 
              | (2U == (3U & (IData)(vlTOPp->in)))) 
             | (4U == (7U & (IData)(vlTOPp->in)))) 
            | (8U == (0xfU & (IData)(vlTOPp->in)))) 
           | (0x10U == (0x1fU & (IData)(vlTOPp->in)))) 
          | (0x20U == (0x3fU & (IData)(vlTOPp->in)))) 
         | (0x40U == (0x7fU & (IData)(vlTOPp->in))))) {
        vlTOPp->out = ((0U == (IData)(vlTOPp->in)) ? 0U
                        : ((1U == (1U & (IData)(vlTOPp->in)))
                            ? 0U : ((2U == (3U & (IData)(vlTOPp->in)))
                                     ? 1U : ((4U == 
                                              (7U & (IData)(vlTOPp->in)))
                                              ? 2U : 
                                             ((8U == 
                                               (0xfU 
                                                & (IData)(vlTOPp->in)))
                                               ? 3U
                                               : ((0x10U 
                                                   == 
                                                   (0x1fU 
                                                    & (IData)(vlTOPp->in)))
                                                   ? 4U
                                                   : 
                                                  ((0x20U 
                                                    == 
                                                    (0x3fU 
                                                     & (IData)(vlTOPp->in)))
                                                    ? 5U
                                                    : 6U)))))));
    } else {
        if ((0x80U == (0xffU & (IData)(vlTOPp->in)))) {
            vlTOPp->out = 7U;
        } else {
            if ((0x100U == (0x1ffU & (IData)(vlTOPp->in)))) {
                vlTOPp->out = 8U;
            } else {
                if ((0x200U == (IData)(vlTOPp->in))) {
                    vlTOPp->out = 9U;
                }
                // else, no Assignment to vlTOPp->out, but we cannot get here!
            }
        }
    }

Can I add // verilator lint_off LATCH to these last 2 test cases and raise a PR for review anyway?

Is your code after V3Case? V3Case does the analysis of full case, and could simply set v3warnOff LATCH on the file lines involved after analyzing them.

I have taken the above adivce, thanks.
It does leave a hole where latches will not be detected in complex case statements, but this is better than false positives IMO and this can be addressed in a later refinement. I'll raise a PR now