|
From: <ni...@ly...> - 2018-03-28 20:35:35
|
Hi, I've tried to use always_comb and always_ff consistently in my code, in the hope that it would detect some bugs in my code. However, I've found one unexpected change in behavior when replacing an "always @(*)" by "always_comb". Consider below program popc-bug.vl (population count of a 64-bit value), and note the `ifdef ALWAYS_COMB. I compile and run it as follows: $ iverilog -g2005-sv -Wall popc-bug.vl && ./a.out $ iverilog -g2005-sv -Wall -DALWAYS_COMB popc-bug.vl && ./a.out popc fail: in: 0000000000000001, out: 0000000, ref: 0000001 $ So when always_comb is used in the last process (the others don't matter, it seems), popc(1) produces 0, not 1 like it should. As usual, it could well be my code which is incorrect, but as you see, I got no warnings or errors from the iverilog compiler. Regards, /Niels ----------------8<--------- /* Copyright (C) 2016 Niels Möller This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later version. This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>. */ module popc (input [63:0] x, output [6:0] cnt); /* Dadda tree, weights: * * 64 64 * 21F+1 * 21 22 43 A * 7F 7F+1 * 7 14 8 29 B * 2F+1 4F+2 2F+H * 2 7 9 3 21 C * 2F+1 3F F * 4 6 4 1 15 D * F+1 2F F+1 * 1 4 3 2 1 11 E * F+1 F H * 2 3 2 1 1 9 F * H F H * 1 2 2 1 1 1 8 G */ function [1:0] ha (input [1:0] x); ha = x[0] + x[1]; endfunction // ha function [1:0] fa (input [2:0] x); fa = x[0] + x[1] + x[2]; endfunction // fa reg [21:0] a0; reg [20:0] a1; reg [7:0] b0; reg [13:0] b1; reg [6:0] b2; reg [2:0] c0; reg [8:0] c1; reg [6:0] c2; reg [1:0] c3; reg d0; reg [3:0] d1; reg [5:0] d2; reg [3:0] d3; reg [1:0] e1; reg [2:0] e2; reg [3:0] e3; reg e4; reg f1; reg [1:0] f2; reg [2:0] f3; reg [1:0] f4; reg g2; reg [1:0] g3; reg [1:0] g4; reg g5; genvar i; assign cnt[2:0] = {g2, f1, d0}; assign cnt[6:3] = {g4[0],g3[0]} + {g5, g4[1], g3[1]}; generate for (i = 0; i < 21; i = i + 1) always @(*) {a1[i], a0[i]} = fa(x[3*i+2:3*i]); endgenerate generate for (i = 0; i < 7; i = i + 1) always @(*) begin {b1[i], b0[i]} = fa(a0[3*i+2:3*i]); {b2[i], b1[7+i]} = fa(a1[3*i+2:3*i]); end endgenerate `ifdef ALWAYS_COMB always_comb `else always @(*) `endif begin a0[21] = x[63]; b0[7] = a0[21]; {c1[0], c0[0]} = fa(b0[2:0]); {c1[1], c0[1]} = fa(b0[5:3]); {c1[2], c0[2]} = ha(b0[7:6]); {c2[0], c1[3]} = fa(b1[2:0]); {c2[1], c1[4]} = fa(b1[5:3]); {c2[2], c1[5]} = fa(b1[8:6]); {c2[3], c1[6]} = fa(b1[11:9]); c1[8:7] = b1[13:12]; {c3[0], c2[4]} = fa(b2[2:0]); {c3[1], c2[5]} = fa(b2[5:3]); c2[6] = b2[6]; {d1[0], d0} = fa(c0); {d2[0], d1[1]} = fa(c1[2:0]); {d2[1], d1[2]} = fa(c1[5:3]); {d2[2], d1[3]} = fa(c1[8:6]); {d3[0], d2[3]} = fa(c2[2:0]); {d3[1], d2[4]} = fa(c2[5:3]); d2[5] = c2[6]; d3[3:2] = c3; {e2[0],e1[0]} = fa(d1[2:0]); e1[1] = d1[3]; {e3[0],e2[1]} = fa(d2[2:0]); {e3[1],e2[2]} = fa(d2[5:3]); {e4, e3[2]} = fa(d3[2:0]); e3[3] = d3[3]; {f2[0], f1} = ha(e1); {f3[0], f2[1]} = fa(e2); {f4[0], f3[1]} = fa(e3[2:0]); f3[2] = e3[3]; f4[1] = e4; {g3[0],g2} = ha(f2); {g4[0],g3[1]} = fa(f3); {g5,g4[1]} = ha(f4); end endmodule // popc module main; reg [63:0] x; wire [6:0] cnt; integer i, j; reg [6:0] ref_popc; initial begin for (i = 0; i <= 64; i = i + 1) for (j = i; j <= 64; j = j + 1) begin #((i | j) ? 10 : 0) begin if (i == j && i > 0) begin x = {64{1'b1}}; ref_popc = 7'd64; end else begin x = (64'b1 << j) - (64'b1 << i); ref_popc = j - i; end end #10 begin if (ref_popc !== cnt) begin $display ("popc fail: in: %x, out: %b, ref: %b", x, cnt, ref_popc); $finish_and_return(1); end end end // for (j = i; j <= 64; j = j + 1) end popc test_popc(x, cnt); endmodule -- Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677. Internet email is subject to wholesale government surveillance. |
|
From: Martin W. <ic...@ma...> - 2018-03-31 19:34:16
|
I think the problem is that you are writing to b0 both inside and outside the always_comb statement. Icarus's sensitivity calculation only operates at the word level, not the bit level, so it doesn't detect that you are writing to different bits inside and outside. It just sees you are writing to b0, so removes it from the sensitivity list. It should then have issued an error message, but I think that's one of the things Cary said he hadn't implemented yet. This is of course a bug, but it's not going to be an easy fix. This is why I never tackled implementing always_comb. Niels Möller wrote: > Hi, > > I've tried to use always_comb and always_ff consistently in my code, in > the hope that it would detect some bugs in my code. > > However, I've found one unexpected change in behavior when replacing an > "always @(*)" by "always_comb". Consider below program popc-bug.vl > (population count of a 64-bit value), and note the `ifdef ALWAYS_COMB. > > I compile and run it as follows: > > $ iverilog -g2005-sv -Wall popc-bug.vl && ./a.out > $ iverilog -g2005-sv -Wall -DALWAYS_COMB popc-bug.vl && ./a.out > popc fail: in: 0000000000000001, out: 0000000, ref: 0000001 > $ > > So when always_comb is used in the last process (the others don't > matter, it seems), popc(1) produces 0, not 1 like it should. As usual, > it could well be my code which is incorrect, but as you see, I got no > warnings or errors from the iverilog compiler. > > Regards, > /Niels |
|
From: <ni...@ly...> - 2018-04-06 07:07:27
|
Martin Whitaker <ic...@ma...> writes: > I think the problem is that you are writing to b0 both inside and > outside the always_comb statement. I see. If I move out the assignment to b0[7] (and eliminate the redundant copy via a0[21]) to a separate always @(*) block, tests pass. > Icarus's sensitivity calculation > only operates at the word level, not the bit level, so it doesn't > detect that you are writing to different bits inside and outside. Bit select expressions must be compile-time constants, right? So it shouldn't be too hard to detect this case, when Cary or someone else gets the time to work on it. > This is of course a bug, but it's not going to be an easy fix. This is > why I never tackled implementing always_comb. Detection should be the first step. But to really fix it, could we split words automatically? Say we have a register [7:0] x, and an always_comb block which assigns x[7:4]. Then replace x by two registers x_0 [7:4] and x_1 [3:0] representing subwords. Then each subword should be assigned in at most one always_comb, and one could also warn for subwords which are never assigned. Might make sense to do this splitting also for other types of processes, for warning purposes. I'm not sure what's actually valid, but I would expect that one generally should avoid having multiple processes assigning the same bit (with exceptions for processes that are triggered by disjoint events, e.g., one proces running on rising clock edge and another running on falling clock edge). Regards, /Niels -- Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677. Internet email is subject to wholesale government surveillance. |
|
From: Cary R. <cy...@ya...> - 2018-04-10 04:08:07
|
The real issue is I just reused the routines for @* which as Martin mentioned are word based (per the standard they do not consider a constant select in the sensitivity calculation; even for an array). I don't think it will be too hard to update things to give both the word based behavior for @* and the longest static prefix behavior needed for always_comb/latch. I looked at that some time ago while working on @* fixes before realizing the standard said otherwise.
I am very busy with work right now so it may be a while before I can take a look at fixing this.
Cary
On Friday, April 6, 2018, 12:07:35 AM PDT, Niels Möller <ni...@ly...> wrote:
Martin Whitaker <ic...@ma...> writes:
> I think the problem is that you are writing to b0 both inside and
> outside the always_comb statement.
I see. If I move out the assignment to b0[7] (and eliminate the
redundant copy via a0[21]) to a separate always @(*) block, tests pass.
> Icarus's sensitivity calculation
> only operates at the word level, not the bit level, so it doesn't
> detect that you are writing to different bits inside and outside.
Bit select expressions must be compile-time constants, right? So it
shouldn't be too hard to detect this case, when Cary or someone else
gets the time to work on it.
> This is of course a bug, but it's not going to be an easy fix. This is
> why I never tackled implementing always_comb.
Detection should be the first step. But to really fix it, could we split
words automatically? Say we have a register [7:0] x, and an always_comb
block which assigns x[7:4]. Then replace x by two registers x_0 [7:4]
and x_1 [3:0] representing subwords. Then each subword should be assigned
in at most one always_comb, and one could also warn for subwords which
are never assigned.
Might make sense to do this splitting also for other types of processes,
for warning purposes. I'm not sure what's actually valid, but I would
expect that one generally should avoid having multiple processes
assigning the same bit (with exceptions for processes that are triggered
by disjoint events, e.g., one proces running on rising clock edge and
another running on falling clock edge).
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Iverilog-devel mailing list
Ive...@li...
https://lists.sourceforge.net/lists/listinfo/iverilog-devel
|