Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect and warn appropriately on intentional latches #1609

Open
veripoolbot opened this issue Nov 19, 2019 · 20 comments
Open

Detect and warn appropriately on intentional latches #1609

veripoolbot opened this issue Nov 19, 2019 · 20 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Nov 19, 2019


Author Name: Julien Margetts
Original Redmine Issue: 1609 from https://www.veripool.org

Original Assignee: Julien Margetts


According to Cummings:
"Guideline #2: When modeling latches, use nonblocking assignments."

However, as I'm sure you are aware, Verilating the following code results in Warning-COMBDLY

module RTLPRIM_LATCH
(
     input  wire D,
     input  wire EN,
     output reg  Q
);
     always @(D or EN)
         if (!EN)
             Q <= D;
endmodule

/* verilator lint_off COMBDLY */ does work (as does using always_latch) but we are not permitted to modify this source

-Wno-COMBDLY also works but I don't want to turn this warning off globally

The following config file also works:

`verilator_config
lint_off -msg COMBDLY -file "*/rtlprim_latch.v"

I'm happy to use the config file as it allows me to re-enable to warning, but is this the recommended aproach?

The reason I ask is that in the warning text:
"%Warning-COMBDLY: test.v:9: Delayed assignments (<=) in non-clocked (non flop or latch) block: ... Suggest blocking assignments (=)"

"non flop or latch" Implies verilator did not recognise this was an intentional latch.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 20, 2019


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 20, 2019


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 :)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 20, 2019


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

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 5, 2019


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 5, 2019


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 15, 2019


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

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 17, 2019


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

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 18, 2019


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.

@wsnyder wsnyder changed the title Detect and warn appripriately on intentional latches Detect and warn appropriately on intentional latches Jan 10, 2020
@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 13, 2020

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?

@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 13, 2020

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

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Jan 13, 2020

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.

@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 15, 2020

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

@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 15, 2020

Am I able to push my branch?

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Jan 15, 2020

Let me review again and run on some private designs.

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Jan 15, 2020

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

@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 15, 2020

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.

@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 16, 2020

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

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Jan 17, 2020

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.

@margej

This comment has been minimized.

Copy link

@margej margej commented Jan 17, 2020

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?

@wsnyder

This comment has been minimized.

Copy link
Member

@wsnyder wsnyder commented Jan 17, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.