steveicarus / iverilog

Icarus Verilog
https://steveicarus.github.io/iverilog/
GNU General Public License v2.0
2.8k stars 521 forks source link

icarus gets caught in a infinite combi loop and I'm not sure why. #1145

Closed sean-galloway closed 3 weeks ago

sean-galloway commented 2 months ago

I'm trying to write a simple and stupid APB crossbar. The attached file is primarily a combi-only block except for the arbiter. I have used the arbiter in many other places with Icarus, so that is not the issue. I also made a heavyweight version that has all flops on both the slave and master interfaces, and that has the same problem. Finally, after finding nothing wrong with the code, I ran it on vcs. The thin and the heavy versions both run fine with that simulator. I believe there is something obvious in one of the generate loops that Icarus doesn't like.

I'm running it with pytest, and this is the iverilog command given: iverilog -o /home/sean/github/RTLDesignSherpa/val/unit_axi/local_sim_build/test_apb_xbar_thin-3-6-32-32/apb_xbar_thin_wrap_m10_s10.vvp -D COCOTB_SIM=1 -g2012 -s apb_xbar_thin_wrap_m10_s10 -Papb_xbar_thin_wrap_m10_s10.M=3 -Papb_xbar_thin_wrap_m10_s10.S=6 -Papb_xbar_thin _wrap_m10_s10.ADDR_WIDTH=32 -Papb_xbar_thin_wrap_m10_s10.DATA_WIDTH=32 -s iverilog_dump /home/sean/github/RTLDesignSherpa/rtl/common/arbiter_fixed_priority.sv /home/sean/github/RTLDesignSherpa/rtl/common/arbiter_round_robin_subinst.sv /home/sean/github/RTLDesignSherpa/rtl/common/arbiter_weighted_round_robin.sv /home/s ean/github/RTLDesignSherpa/rtl/axi/apb_xbar_thin.sv /home/sean/github/RTLDesignSherpa/rtl/integ_axi/apb_xbar/apb_xbar_thin_wrap_m10_s10.sv /home/sean/github/RTLDesignSherpa/val/unit_axi/local_sim_build/test_apb_xbar_thin-3-6-32-32/iverilog_dump.v

apb_xbar_thin.sv.txt

martinwhitaker commented 2 months ago

Without the other design files, I can't reproduce this.

sean-galloway commented 2 months ago

Here are all of the files. issue.tar.gz

sean-galloway commented 2 months ago

I missed one file. Here is the ful batch issue.tar.gz

martinwhitaker commented 2 months ago

Without a Verilog testbench I can't reproduce the infinite loop. But a couple of observations. When compiling there are warnings of the form

sorry: constant selects in always_* processes are not currently supported (all bits will be included).

This means your generated always_comb blocks are going to be re-evaluated more often than is really necessary. Also I see you use the coding style

signal = 0;
if (condition) signal = 1;

This is dangerous in combinatorial logic because it introduces a glitch on signal when the always_comb block is re-evaluated while condition remains true. That glitch can trigger re-evaluation of other blocks.

You should also look at the warnings about port width mismatch.

caryr commented 3 weeks ago

To verify if this is an issue in the constant select sensitivity you can move the constant selects outside the always_comb blocks like the following to get the expected sensitivity:

diff --git a/orig.v b/apb_xbar_thin.sv
index df64c83..17406f8 100644
--- a/orig.v
+++ b/apb_xbar_thin.sv
@@ -72,13 +72,20 @@ module apb_xbar_thin #(

     generate
         for (genvar s_dec = 0; s_dec < S; s_dec++) begin : gen_decoder
+            logic [M-1:0] lms;
+            logic se;
+            logic [ADDR_WIDTH-1:0] sab, sal;
+            assign master_sel[s_dec] = lms;
+            assign se = SLAVE_ENABLE[s_dec];
+            assign sab = SLAVE_ADDR_BASE[s_dec];
+            assign sal = SLAVE_ADDR_LIMIT[s_dec];
             always_comb begin
                 for (int m_dec = 0; m_dec < M; m_dec++) begin
-                    master_sel[s_dec][m_dec] = 1'b0;
-                    if (m_apb_psel[m_dec] && SLAVE_ENABLE[s_dec] &&
-                            (m_apb_paddr[m_dec] >= SLAVE_ADDR_BASE[s_dec]) &&
-                            (m_apb_paddr[m_dec] <= SLAVE_ADDR_LIMIT[s_dec])) begin
-                        master_sel[s_dec][m_dec] = 1'b1;
+                    lms[m_dec] = 1'b0;
+                    if (m_apb_psel[m_dec] && se &&
+                            (m_apb_paddr[m_dec] >= sab) &&
+                            (m_apb_paddr[m_dec] <= sal)) begin
+                        lms[m_dec] = 1'b1;
                         $fdisplay(file, "Decode: Time=%0t s_dec=%0h m_dec=%0h", $realtime/1e3, s_dec, m_dec);
                     end
                 end

Since this uses cocotb based testing and we are not set up to support that we cannot actually run this to test further. Can you make the kind of changes shown above in all the appropriate places to see if this is because of the sensitivity limitation. I've never run cocotb, so I'm not certain if it is hiding the warning/sorry messages. You can get them by just running iverilog with all the .sv files.

caryr commented 3 weeks ago

There is also the file/line tracing that can be enabled to figure out infinite loops in VVP. It only traces procedural code, but that's likely where the loop is located.

caryr commented 3 weeks ago

You could also replace an always_comb with an assign like the following if appropriate:

@@ -172,9 +181,7 @@ module apb_xbar_thin #(
     generate
         for (genvar m = 0; m < M; m++) begin : gen_arb_gnt_mst_m
             for (genvar s = 0; s < S; s++) begin : gen_arb_gnt_mst_s
-                always_comb begin
-                    arb_gnt_mst[m][s] = arb_gnt[s][m];
-                end
+                assign arb_gnt_mst[m][s] = arb_gnt[s][m];
             end
         end
     endgenerate
sean-galloway commented 3 weeks ago

Hi, Sorry, I need to update the issue. When I moved to the most recent version of Icarus, the code runs fine. -sean

On Aug 26, 2024, at 8:27 AM, Cary R. @.***> wrote:



You could also replace an always_comb with an assign like the following if appropriate:

@@ -172,9 +181,7 @@ module apb_xbar_thin #( generate for (genvar m = 0; m < M; m++) begin : gen_arb_gnt_mst_m for (genvar s = 0; s < S; s++) begin : gen_arb_gnt_mst_s

— Reply to this email directly, view it on GitHubhttps://github.com/steveicarus/iverilog/issues/1145#issuecomment-2310485228, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AD7UE5BPRFVVOHQUT7FFGNTZTNCOTAVCNFSM6AAAAABK3O6PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJQGQ4DKMRSHA. You are receiving this because you authored the thread.Message ID: @.***>

caryr commented 3 weeks ago

It's good to know you now have an environment that is working as expected.

sean-galloway commented 3 weeks ago

Thank you so much for all of the hard work on this endeavor. Do you have a "buy me a coffee" or a donate link? I knew icarus was supported, but I didn't realize how active the development was.

-sean


From: Cary R. @.> Sent: Monday, August 26, 2024 9:26 AM To: steveicarus/iverilog @.> Cc: sean galloway @.>; Author @.> Subject: Re: [steveicarus/iverilog] icarus gets caught in a infinite combi loop and I'm not sure why. (Issue #1145)

It's good to know you now have an environment that is working as expected.

— Reply to this email directly, view it on GitHubhttps://github.com/steveicarus/iverilog/issues/1145#issuecomment-2310597214, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AD7UE5BBGS7JVAFNU6GGGYLZTNJL5AVCNFSM6AAAAABK3O6PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJQGU4TOMRRGQ. You are receiving this because you authored the thread.Message ID: @.***>

caryr commented 3 weeks ago

I wouldn't say active development right now since most of us are too busy with our day job changing the world in other ways. What most of us try to do is see if we can help people work around the current limitation/bugs and when we have time available we do try to fix critical bugs.

sean-galloway commented 3 weeks ago

I understand volunteer projects. However, my "old" version of Icarus that had the issue was only a year old, so I think it was more active than I expected.

-sean


From: Cary R. @.> Sent: Monday, August 26, 2024 12:22 PM To: steveicarus/iverilog @.> Cc: sean galloway @.>; Author @.> Subject: Re: [steveicarus/iverilog] icarus gets caught in a infinite combi loop and I'm not sure why. (Issue #1145)

I wouldn't say active development right now since most of us are too busy with our day job changing the world in other ways. What most of us try to do is see if we can help people work around the current limitation/bugs and when we have time available we do try to fix critical bugs.

— Reply to this email directly, view it on GitHubhttps://github.com/steveicarus/iverilog/issues/1145#issuecomment-2310910310, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AD7UE5CNQME57IOZS2HO7OLZTN6AFAVCNFSM6AAAAABK3O6PLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJQHEYTAMZRGA. You are receiving this because you authored the thread.Message ID: @.***>

caryr commented 3 weeks ago

People always seem to want to have a released version, which I do understand, but with Icarus you can almost always just use the latest from the head of git development and have no issues. Occasionally there are short period of instability, but if you want the best we have to offer compiling your own from git is the best choice if you can handle building it yourself.