slang-netlist new feature: Detect combinatorial loops #984

udif · 2024-05-05T16:12:20Z

The following PR adds combinatorial loop detection to slang-netlist.
The original algorithm can be found here:

https://epubs.siam.org/doi/10.1137/0204007
Finding All the Elementary Circuits of a Directed Graph
Johnson, Donald B
SIAM Journal on Computing Vol. 4, Issue. 1 Mar 1975

I have taken a Java implementation from https://github.com/josch/cycles_johnson_meyer , forked it to modernize the Java, then ported the code to C++ and finally modified my generic C++ code to work with @jameshanlon 's code.

While the basic code works, there are lots of things that can be improved:

Report combinatorial path in a more standardized way, using slang's SourceLocation and SourceManager classes.
improve memory usage.
Integrate with slang's diagnostic subsystem, if possible.

Here is a sample input and output:

% cat test.v
module test (input clk);
wire a;
wire b;
wire c;
wire d;

t2 t2(
  .clk(clk),
  .x(a),
  .y(b),
  .z(c)
);
assign a = b & c;
endmodule

module t2(input clk, input x, output y, output reg z);
assign y = x;
always @(posedge clk)
  z <= x;
endmodule
% build/bin/slang-netlist --comb_loops test.v
Top level design units:
    test

Build succeeded: 0 errors, 0 warnings
Nodes: 29
Detected 1 combinatorial loop:
Variable declaration: test.a => a (Assigned to) => Variable alias test.t2.x => x => y (Assigned to) => Variable alias test.t2.y => b => Variable declaration: test.b => b => a (Assigned to)

…y an edge.

2. Added original author's license file.

codecov · 2024-05-05T16:25:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.21%. Comparing base (ddc950d) to head (3bed0dc).
Report is 77 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #984      +/-   ##
==========================================
+ Coverage   94.01%   94.21%   +0.20%     
==========================================
  Files         189      191       +2     
  Lines       48479    46903    -1576     
==========================================
- Hits        45576    44189    -1387     
+ Misses       2903     2714     -189

see 147 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f37bea...3bed0dc. Read the comment docs.

jameshanlon

Looks good.

I'm not sure about including code with a different license that's not explicitly 'third party' but @MikePopoloski can comment.

Also - can you add a unit test for the change?

jameshanlon · 2024-05-05T20:36:21Z

tools/netlist/CMakeLists.txt

@@ -3,7 +3,8 @@
 # SPDX-License-Identifier: MIT
 # ~~~

-add_executable(slang_netlist netlist.cpp source/Netlist.cpp)
+add_executable(slang_netlist netlist.cpp source/Netlist.cpp
+                             source/CombLoops.cpp)


It would be better to include this source in the netlist library, so that the functionality is not only bound to the command-line tool.

I'm not sure I understand what you mean.
The file is already under tools\netlist\source, the same place Netlist.cpp is in, and all its functionality is only included in the command-line tools, not part of libsvlang.a, and I don't see you're building any standalone Netlist library either.
Feel free to fix this file, or show me what you mean.

Ignore me, I thought that I had set up a library!

jameshanlon · 2024-05-05T20:37:30Z

tools/netlist/include/NetlistVisitor.h

+    auto& edge = netlist.addEdge(sourceVarNode, targetVarNode);
+    targetVarNode.edgeKind = edgeKind;


It would be neater to extend the method to include edgeKind as an argument.

addEdge is currently a DirectedGraph method, that has nothing to do with EdgeKind.
Do you want me to override this with a new Netlist::addEdge(NetlistNode& sourceNode, NetlistNode& targetNode, ast::EdgeKind edgeKind) ?

It was a minor point so it's up to you. But that sounds reasonable.

I added it anyway, learning on the way the importance of using when redefining functions on derived classes, and still wanting to call the base class version on the default function signature.

tools/netlist/netlist.cpp

udif · 2024-05-05T20:56:51Z

At the moment I'm also debugging another issue. I use ultraembedded's JPEG core for tests. I can run it against the whole core, or subsets of it. One subset with <700 nodes passes almost immediately. When I moved to a different subtree with ~2000 nodes, the program dies after a few minutes with a segmentation fault. I don't know if this is a complexity issue, or a deadlock triggered by the specific netlist, because it jumps from a sub-second run, to dying after a few minutes.

jameshanlon · 2024-05-05T21:10:48Z

I noticed while looking at #985 that it takes a long time to process the netlist, so this may be related.

$ time ./build/bin/slang-netlist ../core_jpeg/src_v/jpeg_dht*
Top level design units:
    jpeg_dht

Build succeeded: 0 errors, 0 warnings

real    0m6.035s
user    0m6.025s
sys     0m0.020s

If you run with --debug you can see it spends a lot of time looking up variables that appear in the LUTs, eg:

NetlistVisitor.h:70: Edge decl lookup_input_i to ref lookup_input_i
NetlistVisitor.h:83: Edge ref lookup_input_i to ref y_ac_width_r
NetlistVisitor.h:70: Edge decl lookup_input_i to ref lookup_input_i
NetlistVisitor.h:83: Edge ref lookup_input_i to ref y_ac_width_r
...
netlist.cpp:227: Netlist has 1131 nodes and 54233 edges

It needs some more investigation as to why there are so many edges being created.

…ath instead. At the moment we dump the 1st cycle element again at the end of the loop just to close it, but we will probably drop it.

2. Decided not to print 1st element again at the end (it was supposed to close the cycle, but looks redunant).

… edgeKind

udif · 2024-05-06T16:19:22Z

I just added a test (a single one, at least for the moment) to cover CombLoops.
From my side I think the code is ready.

likeamahoney · 2024-05-06T16:22:28Z

As proof of concept is good but i suppose that analysis is need to be more conservative. It seems to me that checking the graph on presence of cycles (without outermost verilog context) is not quite enough for a good combinational(combinatorial) logic loops analysis. Because not every assignment belongs to combinational logic. There is also sequential and latch kinds of procedural logic.

For example in this simple code sample there is no combinational logic at all. It follows that there are also no combinational cycles (but PR solution reports that there is at least one combinational loop):

module top (input clk);
        reg a;
        reg b;

        test t1(.x(a), .y(b), .clk(clk));
        test t2(.x(b), .y(a), .clk(clk));

endmodule

module test(input reg x, output reg y, input clk);
        always_latch begin
                if (clk)
                        y <= x;
        end
endmodule

Build succeeded: 0 errors, 0 warnings
Nodes: 30
Actual active Nodes: 30
Detected 1 combinatorial loop:
Path length: 8
1.sv:5:13: note: variable a assigned to
        test t1(.x(a), .y(b), .clk(clk));
                   ^

1.sv:13:9: note: variable x read from
                        y <= x;
                             ^

1.sv:13:4: note: variable y assigned to
                        y <= x;
                        ^

1.sv:5:20: note: variable b read from
        test t1(.x(a), .y(b), .clk(clk));
                          ^

1.sv:6:13: note: variable b assigned to
        test t2(.x(b), .y(a), .clk(clk));
                   ^

1.sv:13:9: note: variable x read from
                        y <= x;
                             ^

1.sv:13:4: note: variable y assigned to
                        y <= x;
                        ^

1.sv:6:20: note: variable a read from
        test t2(.x(b), .y(a), .clk(clk));

I think it needs to be determined first which assignments are related to combinational logic but which aren't to make analysis more context sensitivity.

udif · 2024-05-06T18:31:55Z

From my experience, 99% of the designs do not use latches, except for clock gating cells, therefore if you encounter a latch it is more often than not a design mistake rather than intention (although with always_comb and always @* we see less and less the plain old always @(a or b or c..) style that leads to unintentional latches).
Even when you do use latches, I don't think you can easily analyze the latch control (by a simple algorithm) to determine whether it is a combinatorial loop or not.

From a practical point of view, since I cannot safely determine when a latch is safe, I would rather have a false positive I can waive (we'll have to add a mechanism for that) rather than skip a warning.

MikePopoloski

This is pretty cool, thanks for the contribution. I'd like the slang library to properly have support for comb loop detection at some point in the future but it first needs to gain another layer of lowering support (possibly to something MLIR based) before it will be feasible to implement. In the meantime it seems good to experiment with doing it in slang-netlist.

MikePopoloski · 2024-05-06T21:06:53Z

tools/netlist/include/CombLoops.h

+#ifndef COMBLOOPS_H
+#define COMBLOOPS_H
+
+/*


IANAL but: I think it's good to leave the original license here but since you translated the source it's now also copyrighted by you and so it's fine to also put the standard license header on this file (and we don't need to put it in external/).

I gave this a lot of thought, and it's like the Ship of Theseus. The original code is not here, but the class names, internal functions and their internal structures (loops, branches, etc.) is mostly the same. Given the similarity between Java and C++ this is even more obvious.
Since the original code was governed by a very permissive BSD-2 clause license, I didn't find this an issue.
Apparently, the original code is really from here: http://www.normalisiert.de/ .

In any case, the code is recursive, and I found that for some cases it dies even for a small number of nodes.
I wish to investigate this for a few more days. From a quick peek at the offending function, I think it might be not too complex to remove the recursion.

MikePopoloski · 2024-05-06T21:09:21Z

Let me know when you think this is ready to land.

likeamahoney · 2024-05-07T11:04:13Z

From my experience, 99% of the designs do not use latches, except for clock gating cells, therefore if you encounter a latch it is more often than not a design mistake rather than intention (although with always_comb and always @* we see less and less the plain old always @(a or b or c..) style that leads to unintentional latches).

Even when you do use latches, I don't think you can easily analyze the latch control (by a simple algorithm) to determine whether it is a combinatorial loop or not.

From a practical point of view, since I cannot safely determine when a latch is safe, I would rather have a false positive I can waive (we'll have to add a mechanism for that) rather than skip a warning.

Modern designs are't use latches it's true but what about D-triggers/flip-flops (which widely used in modern designs to separate combinational logic) and other sequential logics? Provided solution also warns on it.

For example there is simple flip-flop without comb loops:

module top (input rst, input clk);
        wire D;
        wire Q;
        wire Qn;
        dff d1(D, clk, rst, Q, Qn);
        dff d2(Q, clk, rst, D, Qn);
endmodule

module dff (
  input  logic D, clk, rst,
  output logic Q, Qn
);
  always_ff @(posedge clk, posedge rst) begin
    if (rst) begin
      Q  <= 0;
      Qn <= 1;
    end else begin
      Q  <= D;
      Qn <= ~D;
    end
  end
endmodule

udif · 2024-05-07T11:28:02Z

Modern designs are't use latches it's true but what about D-triggers/flip-flops (which widely used in modern designs to separate combinational logic) and other sequential logics? Provided solution also warns on it.

This is simple - your example simply triggered a bug. This was not supposed to happen...
The idea was to disconnect any node below always or always_ff statements with posedge or negedge in their sensitivity list.
Apparently, as your example shows, there are bugs. I will check this.

However, I have found more serious false positive issues:

module t;
wire x, y, z;
assign {z, y} = {~y, ~x};
endmodule

This will also trigger a comb loop warning. While this specific one might be solved (at the cost of complicating the netlist logic) It looks like the general problem will not be solved without a full synthesis engine. For example:

module t;
wire x, y;
assign z = ~y & y;
assign y = z;
endmodule

It remains to be seen if the scope of this feature would be useful given the current limitations (known possible false positives).

jameshanlon · 2024-05-07T13:13:24Z

I would expect the netlist graph for assign {z, y} = {~y, ~x}; to be acyclic, so there may be a bug if that is not true.

I see your point though and agree that without full synthesis it's not possible to determine a true combinatorial loop.

The class of loops that slang-netlist can report are comparable to those reported by Verilator as UNOPTFLAT`, which are still useful to know about.

udif · 2024-05-07T15:50:09Z

I would expect the netlist graph for assign {z, y} = {~y, ~x}; to be acyclic, so there may be a bug if that is not true.

A combinatorial loop is reported because each of the x and y nodes have edges to z and y, as {z, y} is lumped into one assigment depending on y and x and is then split. slang-netlist is not smart enough to detect the bitwise assignment and separate the z = ~y and `y = ~x' assignments.

Maybe we should rename the option name to --possible-comb-loop so that people set the correct expectations, and accept a small number of false positives.

jameshanlon · 2024-05-07T16:06:44Z

slang-netlist is not smart enough to detect the bitwise assignment and separate the z = ~y and `y = ~x' assignments.

Apologies - I thought I had implemented that. My aim for slang-netlist is to provide a datastructure that resolves all source-level connectivity to the bit level. Handling packed vectors is important for that, so I'll try and look into it.

With regards to combinatorial loops, how about just calling the option --report-cycles and some description that these can potentially be combinatorial loops?

udif · 2024-05-07T16:49:10Z

With regards to combinatorial loops, how about just calling the option --report-cycles and some description that these can potentially be combinatorial loops?

But I'm not merely reporting cycles - I'm actively ignoring netlist edges that terminate on a posedge/negedge nodes.

BTW, another feature I would like to implement at a later stage is an improvement to the path reporting mode, that will also report the number of clock edges between the source and destination paths. It would require defining the clock signals, and keeping a table of clock aliases, either those directly connected, or those passing through a clock gating module (you would define the clock gating module name, and the input/output ports).
Today I'm running into situations where I have a memory access bus running through several sample stages across my SoC, and having something that counts the total latency would be useful for me.

jameshanlon · 2024-05-07T17:10:08Z

But I'm not merely reporting cycles - I'm actively ignoring netlist edges that terminate on a posedge/negedge nodes.

Marking sequential edges is a generally useful feature of the tool, since it allows the netlist to be constrained to find combinatorial paths. This is how I implemented a previous similar tool.

BTW, another feature I would like to implement at a later stage is an improvement to the path reporting mode, that will also report the number of clock edges between the source and destination paths.

Nice, that would be useful.

(Btw. I've got no objections to this landing! We can always iron details out later.)

We still detect "posedge x or negedge x" as edge even though this is effecetively combinatorial, because we are lazy.

And renamed previous templates to be more consistent

…g constant expressions out of loops

udif · 2024-05-10T16:07:21Z

This is simple - your example simply triggered a bug.

I've fixed the bug, but I still got 2 issues:

For some tests, the code seems to be stuck in a loop. I need to dig deeper into the algorithm to understand what is the issue. I may dump the test case into a DOT file and run against the original Java code to see if it's a conversion issue or a bug in the original code.
I also have a memory corruption bug that causes my unit tests to fail when I add more than one.

It will take me a few more days to resolve these.

When creating more than one Netlist consecutively, the starting Node ID is not reset back to 1! (Because it is initialized statically). So When creating an Adjacency list we need to subtract the ID of node 0, not just "1".

udif · 2024-05-12T21:34:10Z

It seems the bugfix above (2c57b7d) solved all stability and corruption issues, but when running against the full core_jpeg example I've used before, I ran into #993 .

1. Removed dynamic allocation (new/delete) and replaced it by 1 static buffer 2. Removed tail recursion and replaced it by a loop 3. Removed default namespace std and added explicit std:: prefixes

udif · 2024-05-15T05:58:18Z

At the moment, My code works great under clang-sanitize, where I've debugged it, but for some reasons fails under gcc-11, where there seems to be a memory corruption causing a segment violation as well as locking up the watch windows on vscode.
I'm looking into this.

udif · 2024-05-16T06:07:57Z

While I've fixed the crashes, it seems there are still other issues (lockups on some examples).

udif · 2024-05-16T06:31:43Z

While I've fixed the crashes, it seems there are still other issues (lockups on some examples).

False alarm, I was just being impatient and stopped the test after a few seconds. It ended up taking a few more seconds than what I estimated.

Let me know when you think this is ready to land.

@MikePopoloski @jameshanlon I think it is ready from my side.

udif and others added 5 commits April 30, 2024 23:31

DOT files now contains edge information on each node that is driven b…

bc6f5ae

…y an edge.

Initial working version!

2bab6e6

Ignore disabled edges

6f1c36e

1. Removed dead code

d4914a1

2. Added original author's license file.

style: pre-commit fixes

fd21673

udif and others added 2 commits May 5, 2024 21:02

report number of nodes after edge triggered nodes reduction

addda10

style: pre-commit fixes

4be532e

udif mentioned this pull request May 5, 2024

slang-netlist assertion failure #985

Closed

jameshanlon reviewed May 5, 2024

View reviewed changes

udif added 5 commits May 6, 2024 07:26

removed dead code

9b7798a

Added method to clear NetlistPath so we can reuse it in a loop.

162689e

Removed old dump routine from ElementaryCycleSearch and used NetlistP…

54a3323

…ath instead. At the moment we dump the 1st cycle element again at the end of the loop just to close it, but we will probably drop it.

1. Streamlined printout code

57ad1d7

2. Decided not to print 1st element again at the end (it was supposed to close the cycle, but looks redunant).

--comb_loops => --comb-loops

eab5af3

udif force-pushed the pr_detect_comb_loops branch from f7f4fe1 to eab5af3 Compare May 6, 2024 06:33

pre-commit-ci bot and others added 4 commits May 6, 2024 06:33

style: pre-commit fixes

06c0e27

override addEgde in Netlist class to add a version where you also set…

7e89687

… edgeKind

Added one test (for the moment) covering CombLoops

1305093

style: pre-commit fixes

9f5ada4

udif added 3 commits May 6, 2024 21:51

Merge branch 'master' into pr_detect_comb_loops

9554756

Fix breakage caused by MikePopoloski#986

e393316

release allocated memory

c13fd5a

MikePopoloski reviewed May 6, 2024

View reviewed changes

udif and others added 5 commits May 8, 2024 09:42

Handle edge detection on sensitivity lists with more than one item.

5b1dd50

We still detect "posedge x or negedge x" as edge even though this is effecetively combinatorial, because we are lazy.

replace find() with a shorter template

433e277

Added one more template,

2099662

And renamed previous templates to be more consistent

Minor cleanup, mostly removing redundant this-> references, and movin…

2a57ad9

…g constant expressions out of loops

style: pre-commit fixes

aee050b

udif added 2 commits May 12, 2024 23:45

Bug Fix!

2c57b7d

When creating more than one Netlist consecutively, the starting Node ID is not reset back to 1! (Because it is initialized statically). So When creating an Adjacency list we need to subtract the ID of node 0, not just "1".

Enabled all 4 tests for Comb Loops

1e76371

udif and others added 5 commits May 13, 2024 09:09

Further code cleanup

8cf541f

Minor cleanups

244a48b

snapshot

04719ec

Major cleanup:

b76456c

1. Removed dynamic allocation (new/delete) and replaced it by 1 static buffer 2. Removed tail recursion and replaced it by a loop 3. Removed default namespace std and added explicit std:: prefixes

style: pre-commit fixes

d1f314d

udif and others added 2 commits May 16, 2024 08:03

Crash fix!

0026368

style: pre-commit fixes

3bed0dc

MikePopoloski merged commit 75f760b into MikePopoloski:master May 17, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

slang-netlist new feature: Detect combinatorial loops #984

slang-netlist new feature: Detect combinatorial loops #984

udif commented May 5, 2024

codecov bot commented May 5, 2024 •

edited

jameshanlon left a comment

jameshanlon May 5, 2024

udif May 6, 2024

jameshanlon May 6, 2024

jameshanlon May 5, 2024

udif May 6, 2024

jameshanlon May 6, 2024

udif May 6, 2024

udif commented May 5, 2024

jameshanlon commented May 5, 2024 •

edited

udif commented May 6, 2024

likeamahoney commented May 6, 2024

udif commented May 6, 2024

MikePopoloski left a comment

MikePopoloski May 6, 2024

udif May 6, 2024

MikePopoloski commented May 6, 2024

likeamahoney commented May 7, 2024

udif commented May 7, 2024

jameshanlon commented May 7, 2024 •

edited

udif commented May 7, 2024

jameshanlon commented May 7, 2024

udif commented May 7, 2024

jameshanlon commented May 7, 2024

udif commented May 10, 2024

udif commented May 12, 2024

udif commented May 15, 2024

udif commented May 16, 2024

udif commented May 16, 2024

		auto& edge = netlist.addEdge(sourceVarNode, targetVarNode);
		targetVarNode.edgeKind = edgeKind;

slang-netlist new feature: Detect combinatorial loops #984

slang-netlist new feature: Detect combinatorial loops #984

Conversation

udif commented May 5, 2024

codecov bot commented May 5, 2024 • edited

Codecov Report

jameshanlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udif commented May 5, 2024

jameshanlon commented May 5, 2024 • edited

udif commented May 6, 2024

likeamahoney commented May 6, 2024

udif commented May 6, 2024

MikePopoloski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikePopoloski commented May 6, 2024

likeamahoney commented May 7, 2024

udif commented May 7, 2024

jameshanlon commented May 7, 2024 • edited

udif commented May 7, 2024

jameshanlon commented May 7, 2024

udif commented May 7, 2024

jameshanlon commented May 7, 2024

udif commented May 10, 2024

udif commented May 12, 2024

udif commented May 15, 2024

udif commented May 16, 2024

udif commented May 16, 2024

codecov bot commented May 5, 2024 •

edited

jameshanlon commented May 5, 2024 •

edited

jameshanlon commented May 7, 2024 •

edited