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

shift self unproven circuit #2728

Closed
drom opened this issue Apr 13, 2021 · 6 comments · Fixed by #3502 · May be fixed by #2740
Closed

shift self unproven circuit #2728

drom opened this issue Apr 13, 2021 · 6 comments · Fixed by #3502 · May be fixed by #2740
Assignees
Labels
discuss to be discussed at next dev jour fixe (see #devel-discuss at https://yosyshq.slack.com/)

Comments

@drom
Copy link

drom commented Apr 13, 2021

Steps to reproduce the issue

The following Verilog have: ERROR: Found 12 unproven $equiv cells in 'equiv_status -assert'. with itself.

module top_mod(
  input         clock,
  input         arst,
  input  [16:0] tmp4,
  output [11:0] tmp29
);
  reg  state_11;
  wire  next_11 = state_11 + 1'h1;
  assign tmp29 = tmp4 >>> next_11;
  always @(posedge clock) begin
    if (arst) begin
      state_11 <= 1'h0;
    end else begin
      state_11 <= next_11;
    end
  end
endmodule
VFILE1=a_top_mod_old.sv
VFILE2=a_top_mod_new.sv
DUT=top_mod

yosys -q -p "
  read_verilog $VFILE1
  rename $DUT top1
  proc
  memory
  flatten top1
  hierarchy -top top1
  async2sync
  read_verilog $VFILE2
  rename $DUT top2
  proc
  memory
  flatten top2
  equiv_make top1 top2 equiv
  hierarchy -top equiv
  async2sync
  clean -purge
  equiv_simple -short -undef
  equiv_induct -undef -seq 50
  equiv_status -assert
"

Yosys 0.9+3911 (git sha1 58a57551, clang 11.0.1 -fPIC -Os)

Expected behavior

to be equivalent with itself

ERROR: Found 12 unproven $equiv cells in 'equiv_status -assert'.

Actual behavior

not equivalent to itself

@fabianschuiki
Copy link

The problem disappears if the >>> is replaced with a +, -, /; the problem remains for all other shift operators, comparison operators, as well as *.

@nakengelhardt
Copy link
Member

You really do not want to run clean in this context! If you look at what it actually is doing you will see the problem:

[after running all but the last 4 commands of your script]
yosys> debug clean -purge
Finding unused cells or wires in module \equiv..
  removing unused `$equiv' cell `$auto$equiv_make.cc:280:find_same_wires$15'.
  removing unused `$equiv' cell `$auto$equiv_make.cc:280:find_same_wires$16'.
  removing unused non-port wire \tmp4_gold.
  removing unused non-port wire \clock_gold.
  removing unused non-port wire \arst_gold.
  removing unused non-port wire \tmp4_gate.
  removing unused non-port wire \clock_gate.
  removing unused non-port wire \arst_gate.
  removing unused non-port wire \next_11.
  removing unused non-port wire \state_11.
Removed 2 unused cells and 8 unused wires.

If you leave out the clean command it successfully proves equivalent.

@drom
Copy link
Author

drom commented Apr 14, 2021

Thank you @nakengelhardt I will test proposed solution over more cases.

@drom
Copy link
Author

drom commented Apr 14, 2021

well some tests that where failing with clean now passing, but some other tests that where passing with clean now failing without it. Would be great having some more universal solution.

@clairexen clairexen added the discuss to be discussed at next dev jour fixe (see #devel-discuss at https://yosyshq.slack.com/) label Apr 19, 2021
@nakengelhardt
Copy link
Member

It does look like there is a bug in equiv_make that was introduced with #641; we are investigating further. This circuit used to be provable with equiv_simple before that PR.

clairexen added a commit that referenced this issue Apr 28, 2021
This reverts commit 08be796, reversing
changes made to 38dbb44.

This fixes #2728. PR #641 did not actually "fix" #639.

The actual issue in #639 is not equiv_make, but assumptions in equiv_simple
that are not true for the test case provided in #639.
@nakengelhardt
Copy link
Member

As it turns out, #641 did not in fact fix anything, but instead disabled a necessary step in equiv_make where the users of the gold and gate versions of a signal get connected to the output of the corresponding $equiv cell. This was only masking a less common bug by introducing a worse bug that would affect nearly all designs. What equiv_make was doing in the example in #639 was unintuitive but not incorrect; the problem causing equiv_simple to pass erroneously is a fundamental design flaw in equiv_simple where it works with wrong assumptions (just picking one input to an $equiv cell as value when proving a different one). We'll revert #641 which will fix this test, but unfortunately the original bug from #639 will remain. Fixing that would require a rewrite of equiv_simple, but we have already started working on a different new tool for equivalence checking with yosys which will hopefully be an improvement over this and the other problems with the equiv_* passes.

clairexen added a commit that referenced this issue Jun 18, 2021
This reverts commit 08be796, reversing
changes made to 38dbb44.

This fixes #2728. PR #641 did not actually "fix" #639.

The actual issue in #639 is not equiv_make, but assumptions in equiv_simple
that are not true for the test case provided in #639.
jix pushed a commit to jix/yosys that referenced this issue Sep 13, 2022
This reverts commit 08be796, reversing
changes made to 38dbb44.

This fixes YosysHQ#2728. PR YosysHQ#641 did not actually "fix" YosysHQ#639.

The actual issue in YosysHQ#639 is not equiv_make, but assumptions in equiv_simple
that are not true for the test case provided in YosysHQ#639.
jix pushed a commit to jix/yosys that referenced this issue Oct 7, 2022
This reverts commit 08be796, reversing
changes made to 38dbb44.

This fixes YosysHQ#2728. PR YosysHQ#641 did not actually "fix" YosysHQ#639.

The actual issue in YosysHQ#639 is not equiv_make, but assumptions in equiv_simple
that are not true for the test case provided in YosysHQ#639.
@jix jix closed this as completed in #3502 Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss to be discussed at next dev jour fixe (see #devel-discuss at https://yosyshq.slack.com/)
Projects
None yet
4 participants