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

Co-simulation fails for $fa cell #4638

Open
RCoeurjoly opened this issue Oct 7, 2024 · 7 comments
Open

Co-simulation fails for $fa cell #4638

RCoeurjoly opened this issue Oct 7, 2024 · 7 comments
Labels
pending-verification This issue is pending verification and/or reproduction

Comments

@RCoeurjoly
Copy link
Contributor

RCoeurjoly commented Oct 7, 2024

Version

Yosys 0.45+153 (git sha1 b3b88e5, g++ 13.2.0 -Og -fPIC)

On which OS did this happen?

Linux

Reproduction Steps

  • Move to functional tests dir
    cd tests/functional
  • Generate $fa cell design and execute the C++ functional backend
    ../../yosys -p "test_cell -n 1 -w test_cell \$fa; read_rtlil test_cell_fa_00000.il; write_functional_cxx my_module_functional_cxx.cc"
    • Note:
      The generated .il file in my case is the following:
# Generated by Yosys 0.45+153 (git sha1 b3b88e56d, g++ 13.2.0 -Og -fPIC)
autoidx 1
module \gold
  wire input 1 \A
  wire input 2 \B
  wire input 3 \C
  wire output 4 \X
  wire output 5 \Y
  cell $fa \UUT
    parameter \WIDTH 1
    connect \A \A
    connect \B \B
    connect \C \C
    connect \X \X
    connect \Y \Y
  end
end
  • Compile the VCD harness (note the vcd_harness.cc includes my_module_functional_cxx.cc)
    g++ -g vcd_harness.cc -I ../../backends/functional/cxx_runtime/ -std=c++17 -o vcd_harness
  • Execute VCD harness:
    ./vcd_harness test_cell_fa_00000.il_functional_cxx.vcd 100 1
  • Co-simulate
    ../../yosys -p "read_rtlil test_cell_fa_00000.il; sim -r test_cell_fa_00000.il_functional_cxx.vcd -vcd test_cell_fa_00000.il_yosys_sim.vcd -scope gold -timescale 1us -n 100"

Note:

The issue may not be with the functional backend, but with simlib.v or elsewhere.
For example https://github.com/YosysHQ/yosys/blob/main/techlibs/common/simlib.v#L583 seems a bit suspect, since Y ^ Y is always 0.
I think it should be:
assign Y = t1 ^ C, X = (t2 | t3);
but the problem persists even with that change.

Expected Behavior

Co-simulation successful:

Co-simulating sample 99 [99us].

End of script.

Actual Behavior

ERROR: Assert `arg3.bits.size() == 0' failed in ./kernel/celltypes.h:516.

@RCoeurjoly RCoeurjoly added the pending-verification This issue is pending verification and/or reproduction label Oct 7, 2024
@povik
Copy link
Member

povik commented Oct 7, 2024

For example https://github.com/YosysHQ/yosys/blob/main/techlibs/common/simlib.v#L583 seems a bit suspect, since Y ^ Y is always 0.

My guess is this is for faithful x-propagation, i.e. it's to arrange for X to be in x-state whenever Y is in x-state.

@povik
Copy link
Member

povik commented Oct 7, 2024

This issue might be because $fa lacks an eval implementation, similar to #3913 for the case of $alu

@aiju
Copy link
Contributor

aiju commented Oct 8, 2024

I can confirm this is not about the functional backend at all, it's the lack of an eval implementation. I ran into this while writing the tests for the functional backend, the actual functional backend tests work around this issue by using one of the yosys passes to map $fa and $alu to other cells before co-simulation.

@RCoeurjoly RCoeurjoly changed the title functional backend: Co-simulation fails for $fa cell Co-simulation fails for $fa cell Oct 8, 2024
@RCoeurjoly
Copy link
Contributor Author

This issue might be because $fa lacks an eval implementation, similar to #3913 for the case of $alu

yes, also the $lcu eval implementation seems to be missing too.

@nakengelhardt
Copy link
Member

$fa and $lcu are usually very temporary intermediate steps of techmapping, so we've generally not bothered to add support to various passes for them. Have you come across a context where it would actually be necessary or useful to simulate them?

@RCoeurjoly
Copy link
Contributor Author

$fa and $lcu are usually very temporary intermediate steps of techmapping, so we've generally not bothered to add support to various passes for them. Have you come across a context where it would actually be necessary or useful to simulate them?

Not really. Maybe then we could mark $fa, $alu and $lcu as not evaluable?

@povik
Copy link
Member

povik commented Oct 8, 2024

Maybe then we could mark $fa, $alu and $lcu as not evaluable?

Note evaluability is currently conflated with the cell being combinational, see #4034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-verification This issue is pending verification and/or reproduction
Projects
None yet
Development

No branches or pull requests

4 participants