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

Warn/Error code with X, Z values use #5115

Open
algrobman opened this issue May 11, 2024 · 26 comments
Open

Warn/Error code with X, Z values use #5115

algrobman opened this issue May 11, 2024 · 26 comments
Labels
status: asked reporter Bug is waiting for reporter to answer a question

Comments

@algrobman
Copy link

Does verilator warn/error RTL code with X or Z values usage?
We use bunch of arithmetic modules and they have "if" statements with X values like

if(a==1) out = ..
else if (a === 1'bx) out = 'x
else out = ...

And because verilator is 2 state simulator it takes 'if X' branch for zero value of 'a' , producing wrong result at the end.

@algrobman algrobman added the new New issue not seen by maintainers label May 11, 2024
@algrobman
Copy link
Author

right now we have:
-Wno-WIDTH -Wno-MULTIDRIVEN -Wno-UNOPTFLAT -Wno-STMTDLY
to disable some warnings ...

@wsnyder wsnyder added status: asked reporter Bug is waiting for reporter to answer a question and removed new New issue not seen by maintainers labels May 11, 2024
@wsnyder
Copy link
Member

wsnyder commented May 11, 2024

Verilator shouldn't ever consider 0 === 1'bx to be true.

Note it does in a few limited cases know how to X propagate during elaboration, so if you e.g. have a parameter with a value of 1'bx, then PARAM === 1'bx will be true, even though Verilator doesn't do X in a general sense.

So to summarize, we need a small test case ;)

@algrobman
Copy link
Author

Was VCD dumping changed since verilator 4..?

@algrobman
Copy link
Author

I see some discrepancy with Xcelium. for following

logic[7:0] a=1, dec;
always @(posedge clk) begin
    a = a << 1;
end

image

why does 'a' start from 0?

(even originally I had a= 1; in initial statement, but it starts from 0, anyway?

@wsnyder
Copy link
Member

wsnyder commented May 12, 2024

There were some dumping changes. Most likely there's a X=>0/1 transition at time zero causing the posedge to trigger. Thou shall use resets ;)

@algrobman
Copy link
Author

Even if I define "a" as bit vector, nothing changes in waves and printing 'a' value at negedge clk .
(xcellium starts 'a' as 1 , verilator as 0 , bug?

I expect that at first clk rising edge 'a' should become 2, not 1...

BTW, if 'a' would have initial value of 0, "a=a<<1;" statement would never change the 'a' .
(I do not have reset - I test combo logic and 'a' is input to it)

@wsnyder
Copy link
Member

wsnyder commented May 12, 2024

I still suspect a time zero race between the initial and the assignment, with X's. Put a display in the always loop, and see how on Xcelium to get waves at each delta simulation point.

@algrobman
Copy link
Author

BTW, to my original complain about no warnings for operations with X, Z. It's strange that the verilator 5.. started complain about # delays in the code, and does not complain about Xs ...

@algrobman
Copy link
Author

I still suspect a time zero race between the initial and the assignment, with X's. Put a display in the always loop, and see how on Xcelium to get waves at each delta simulation point.

There should not be any race - the waves you see are done on code without initial statement.
the Xcelliun waves look :
image

@wsnyder
Copy link
Member

wsnyder commented May 12, 2024

Because # delay behavior changed relative to older Verilators but X/Z are handled the same.

@wsnyder
Copy link
Member

wsnyder commented May 12, 2024

Oh, you said "I see some discrepancy with Xcelium" I thought you meant the wrong values from Xcelium.

I'd still suggest put a display before and after the assign in the posedge.

@algrobman
Copy link
Author

here $displays for Xcellium:

before a=01
after a=02
a=02, dec=02 enc=6 T=10
before a=02
after a=04
a=04, dec=04 enc=5 T=20
before a=04
..

Here for verilator:

VerilatorTB: Start of sim

a=01, dec=01 enc=7 T=10
before a=01
after a=02
a=02, dec=02 enc=6 T=20
before a=02
after a=04

its crazy - seems verilator misses 1st clk posedge (?)

(I generate clk by C++ code)

@algrobman
Copy link
Author

statements with T= are $display at negadge clk ..

@wsnyder
Copy link
Member

wsnyder commented May 12, 2024

As mentioned above, there's probably a X->0/1 clock edge in Xcelium, but Verilator won't get it. Avoid assuming edges on time 0 events, much better to rely on an explicit reset wire.

@algrobman
Copy link
Author

looks like all this because of my C++ code:

int main(int argc, char** argv) {
  std::cout << "\nVerilatorTB: Start of sim\n" << std::endl;
        
  Verilated::commandArgs(argc, argv);

  Vtb_top* tb = new Vtb_top;

  // init trace dump
  VerilatedVcdC* tfp = NULL;
  
#if VM_TRACE
  Verilated::traceEverOn(true);
  tfp = new VerilatedVcdC;
  tb->trace (tfp, 24);
  tfp->open ("sim.vcd");
#endif
  // Simulate
  while(!Verilated::gotFinish()){
#if VM_TRACE
      tfp->dump (main_time);
#endif
      main_time += 5;
      tb->clk = !tb->clk;
      tb->eval();
  }

#if VM_TRACE
  tfp->close();
#endif

  std::cout << "\nVerilatorTB: End of sim" << std::endl;
  exit(EXIT_SUCCESS);

}

it does not evaluate design before the clock change at time 0 :(

@algrobman
Copy link
Author

one more problem: Cadence/Synopsys tools could not read VCD produced by Verilator.

$version Generated by VerilatedVcd $end
$timescale 1ps $end
 $scope module  $end
  $scope module $unit $end
   $var wire 32 @G$ UVM_LOW [31:0] $end
  $upscope $end

I had to patch it as :

$version Generated by VerilatedVcd $end
$timescale 1ps $end
 $scope module **aaa**  $end
  $scope module **bbb** $end
   $var wire 32 @G$ UVM_LOW [31:0] $end
  $upscope $end

Cadence tool did not show waves, Synopsys DVE complained about line 4 illegal synthax:

Error: [VCD2VPD-PRINT-ERROR] VCD syntax error.
A fatal error occurred at line 4:
Syntax error - $end expected while reading $scope.
The input VCD file contains unsupported syntax that cannot be converted to
VPD.
It could be that the input VCD was generated by a different simulator.
Please check the VCD file and try again.
Error: Conversion from VCD to VPD failed

I guess it was because no name in the $scope statements..

@wsnyder
Copy link
Member

wsnyder commented May 13, 2024

Perhaps you're not creating the Verilated model with a name and that gets passed through? Anyhow, can you please submit a pull request to either name empty scopes, or (not sure this works) filter them out, or if you can't get to a pull, file a new issue with a self-contained test case showing the issue. Thanks.

@algrobman
Copy link
Author

Perhaps you're not creating the Verilated model with a name and that gets passed through?
Not sure I understand your question. My run script/TB/RTL content did not change. I did not see this problem with verilator 4.210, but with 5.024 ...

@wsnyder
Copy link
Member

wsnyder commented May 13, 2024

There have been many tracing changes, if you get a bad file out of the master version, it's a bug.

@algrobman
Copy link
Author

Wilson, thanks again. Now I got to our initial problem - xrun/verilator simulation difference with verilog checking for Xs:

here is the problematic module:

module lzd (a,enc,dec);

parameter a_width  = 8;

  parameter log_a_width = a_width > 536870912 ? 30 :
    (a_width > 268435456   ? 29 :
     (a_width > 134217728   ? 28 :
      (a_width > 67108864   ? 27 :
       (a_width > 33554432   ? 26 :
        (a_width > 16777216   ? 25 :
         (a_width > 8388608   ? 24 :
          (a_width > 4194304   ? 23 :
           (a_width > 2097152   ? 22 :
            (a_width > 1048576   ? 21 :
             (a_width > 524288    ? 20 :
              (a_width > 262144    ? 19 :
               (a_width > 131072    ? 18 :
                (a_width > 65536     ? 17 :
                 (a_width > 32768     ? 16 :
                  (a_width > 16384     ? 15 :
                   (a_width > 8192      ? 14 :
                    (a_width > 4096      ? 13 :
                     (a_width > 2048      ? 12 :
                      (a_width > 1024      ? 11 :
                       (a_width > 512       ? 10 :
                        (a_width > 256       ? 9 :
                         (a_width > 128       ? 8 :
                          (a_width > 64        ? 7 :
                           (a_width > 32        ? 6 :
                            (a_width > 16        ? 5 :
                             (a_width > 8         ? 4 :
                              (a_width > 4         ? 3 :
                               (a_width > 2 ? 2 : 1))))))))))))))))))))))))))));

input [a_width-1:0]  a;
output [log_a_width:0] enc;
output [a_width-1:0]  dec;
reg [log_a_width:0]  enc;
reg [a_width-1:0] dec;
integer i ;

always @ (a)
begin
    enc = {log_a_width+1{1'b1}};   // set enc default to all 1 assuming "A" is all zeros
    dec = {a_width{1'b0}};  // set dec default to all zeros
  begin : block
    for (i=0; (i < a_width); i=i+1) begin
      if (a[a_width-1-i] == 1'b1) begin
      $display("i=%0d", i);
        enc = i;
        dec[a_width-1-i] = 1'b1;
       disable block;  // found first "1", then stop looking
      end // if
      else if (a[a_width-1-i] == 1'bx) begin
      $display("X:i=%0d", i);
        enc = {log_a_width+1{1'bx}};
       disable block;  // when found first "x", then stop looking
      end // else if
    end // for
  end
end
endmodule

here is TB:

`ifdef VERILATOR
module tb_top ( input bit clk );
`else
module tb_top;
    bit                         clk;
`endif
parameter W = 8;
bit [W-1:0] a=1;
wire[W-1:0] dec;
logic [3:0] enc;


always @(negedge clk) begin
    $display("a=%h, dec=%h enc=%0d T=%0d", a, dec , enc, $time);
    if(a == 0) begin
        $finish;
    end
    // End Of test monitor
end

always @(posedge clk) begin
//    $display("before a=%h", a);
    a = a << 1;
//    $display("after a=%h", a);
end

// trace monitor

initial begin
//        a = $random;
`ifndef VERILATOR
        if($test$plusargs("dumpon")) $dumpvars;
        forever  clk = #5 ~clk;
`endif
end

lzd #W lzd(a,enc,dec);


endmodule

C++ wrapper was provided in previous comments.

xrun output :

i=7
i=6
a=02, dec=02 enc=6 T=10
i=5
a=04, dec=04 enc=5 T=20
i=4
a=08, dec=08 enc=4 T=30
i=3
a=10, dec=10 enc=3 T=40
i=2
a=20, dec=20 enc=2 T=50
i=1
a=40, dec=40 enc=1 T=60
i=0
a=80, dec=80 enc=0 T=70
a=00, dec=00 enc=15 T=80

verilator output:

VerilatorTB: Start of sim

X:i=0
X:i=0
a=02, dec=00 enc=0 T=10
X:i=0
a=04, dec=00 enc=0 T=20
X:i=0
a=08, dec=00 enc=0 T=30
X:i=0
a=10, dec=00 enc=0 T=40
X:i=0
a=20, dec=00 enc=0 T=50
X:i=0
a=40, dec=00 enc=0 T=60
i=0
a=80, dec=80 enc=0 T=70
X:i=0
a=00, dec=00 enc=0 T=80
- tb_top.sv:16: Verilog $finish

Based on that seems verilator evaluates if(a[i] == 1'bx) as TRUE and exits the for loop for a[i] ==0 .

Although xrun simulates this correctly.

That is why it's worth to warn compilation of RTL with Xs, at least

@wsnyder
Copy link
Member

wsnyder commented May 13, 2024

There's a lot of code there, is what you intend to ask why:

module t;
   bit [7:0] a;
   initial begin
      a = 0;
      if (a[0] == 1'bx) $display("a=%x == 1'bx //BAD", a);
      if (a[0] != 1'bx) $display("a=%x != 1'bx", a);
      if (a[0] === 1'bx) $display("a=%x === 1'bx //BAD", a);
      if (a[0] !== 1'bx) $display("a=%x !== 1'bx", a);
      a = 1;
      if (a[0] == 1'bx) $display("a=%x == 1'bx //BAD", a);
      if (a[0] != 1'bx) $display("a=%x != 1'bx", a);
      if (a[0] === 1'bx) $display("a=%x === 1'bx //BAD", a);
      if (a[0] !== 1'bx) $display("a=%x !== 1'bx", a);
      $finish;
   end
endmodule

In Verilator should correctly handle or warn on:

a=00 == 1'bx //BAD
a=00 !== 1'bx
a=01 != 1'bx
a=01 !== 1'bx

I'd argue that any == 1'bx probably should most likely be === 1'bx, regardless of simulator, so that's a bit different than requesting a lint warning for Verilator only on every X/Z (which might also be a good idea).

@algrobman
Copy link
Author

You are right about '===' , but we got this code from a third party and warn them about it. Regardless, X,Z usage warning could save a lot of debug time in future. I asked for global warning , thinking it easier to implement, than warn about specific cases.
Another simulation discrepancy could be if X/Z are assigned.
Besides, the warning can be always disabled by -Wno-*

@algrobman
Copy link
Author

Are you going to fix all these issues?

@wsnyder
Copy link
Member

wsnyder commented May 15, 2024

Are you going to fix all these issues?

By "all", I think we're down to general warning on = 'bx/z, and Verilator warning on cases where x/z may be dropped (which is not every assignment). The former is higher priority as is general and more likely problematic (and also what you hit).

By "you", I'm not personally going to get to either soon, pull requests welcome.

@algrobman
Copy link
Author

I meant to add at least warnings for usage of X/Zs and fixing "empty/unnamed" $scope module statements in VCD any soon.

@wsnyder
Copy link
Member

wsnyder commented May 15, 2024

As suggested above, file a separate ticket for $scope, including an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: asked reporter Bug is waiting for reporter to answer a question
Projects
None yet
Development

No branches or pull requests

2 participants