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

Unexpected behaviour of for loop and if statement #1243

Closed
ymherklotz opened this issue Aug 1, 2019 · 4 comments · Fixed by #1267
Closed

Unexpected behaviour of for loop and if statement #1243

ymherklotz opened this issue Aug 1, 2019 · 4 comments · Fixed by #1267
Assignees
Labels
bug fix pending PR with a fix is pending

Comments

@ymherklotz
Copy link

ymherklotz commented Aug 1, 2019

Steps to reproduce the issue

Consider the following piece of code:

module top  (y, clk, sel);
   output wire y  ;
   input       clk; 
   input       sel;
   reg         reg_assign = (1'h0) ;
   reg [1:0]   reg_count = (1'h0) ;
   assign y = reg_assign ;
   always @(posedge clk) 
     if (sel)
       for (reg_count = 0; reg_count < 2; reg_count = reg_count + 1'h1)
         if (0);
         else reg_assign <= 1;
     else reg_assign <=  0;
endmodule

First of all, the for loop in the code does seem a bit dodgy, however, I would still expect reg_assign to be set to 1 when sel is high. When sel is low, reg_assign should then be reset to 0.

However, when synthesised with

Yosys 0.8+618 (git sha1 acd8bc0a, clang  -fPIC -Os)

using

yosys -p "read_verilog rtl.v; synth; write_verilog -noattr synth.v"

reg_assign is set to a constant 0 instead of to what the value of sel is. Removing the dead if statement in the for loop results in the correct behaviour.

I have also attached a folder containing a test bench and SymbiYosys script to compare the design to the synthesised net list.

Expected behaviour

I would expect this to be implemented by assigning sel to y. This is actually also the output of a previous version of Yosys (Yosys 0.8+508 (git sha1 c2ea3746, clang 8.0.0 -fPIC -Os))

module top(y, clk, sel);
  input clk;
  wire reg_assign;
  wire [1:0] reg_count;
  input sel;
  output y;
  assign reg_assign = 1'h0;
  assign reg_count = 2'h0;
  assign y = sel;
endmodule

Actual behaviour

However, with Yosys, y is set to a constant 0.

module top(y, clk, sel);
  input clk;
  wire reg_assign;
  wire [1:0] reg_count;
  input sel;
  output y;
  assign reg_assign = 1'h0;
  assign reg_count = 2'h0;
  assign y = 1'h0;
endmodule

test.zip

@daveshah1
Copy link

Reverting #1169 seems to give the expected result (I don't think assigning sel to y is correct, but the register looks good):

module top(y, clk, sel);
  input clk;
  reg reg_assign = 1'h0;
  wire [1:0] reg_count;
  input sel;
  output y;
  always @(posedge clk)
      reg_assign <= sel;
  assign reg_count = 2'h0;
  assign y = reg_assign;
endmodule

cc @whitequark

@ymherklotz
Copy link
Author

Ah yes you are right, assigning sel directly does not seem correct, clocking it is definitely right.

@whitequark
Copy link
Member

@daveshah1 Good catch, I'll fix the pass in a moment.

@eddiehung eddiehung added the bug label Aug 6, 2019
whitequark added a commit to whitequark/yosys that referenced this issue Aug 8, 2019
Before this commit, in a process like:
   process $proc$bug.v:8$3
     assign $foo \bar
     switch \sel
       case 1'1
         assign $foo 1'1
         assign $foo 1'1
       case
         assign $foo 1'0
     end
   end
both of the "assign $foo 1'1" would incorrectly be removed.

Fixes YosysHQ#1243.
@whitequark
Copy link
Member

Fixed in #1267.

@whitequark whitequark added the fix pending PR with a fix is pending label Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix pending PR with a fix is pending
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants