-
Notifications
You must be signed in to change notification settings - Fork 78
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
counter_test example hits yosys ABC9 assert during synth #120
Comments
Here are the checksums of my build files, I have also attached them in case the build isn't reproducible:
|
@Xiretza I know what is going on here. Yosys built with gcc used in packages builder CI does not call asserts at all - they are optimised out. Here is the part of the code where you hit assert:
When built with the GCC used to build the package it gives the following result:
All the
Here, asserts calls are present. I got similar results with clang. I don't know yet, why the asserts are optimised out. However they seem to be too strict - we can get a working netlist without them afterwards. @Xiretza for now you can simply comment out all the |
What's the |
|
I can confirm the the issue is resolved on my system after commenting out those asserts when I compiled the latest Yosys version |
Alternative fix: --- /tmp/reproduce/install/share/symbiflow/techmaps/xc7_vpr/techmap/cells_sim.v.orig 2021-03-01 18:50:07.403552300 +0000
+++ /tmp/reproduce/install/share/symbiflow/techmaps/xc7_vpr/techmap/cells_sim.v 2021-03-01 18:45:35.903552300 +0000
@@ -85,7 +85,7 @@
endmodule
-(* abc9_box, blackbox *)
+(* abc9_box, lib_whitebox *)
module CARRY4_VPR(O0, O1, O2, O3, CO0, CO1, CO2, CO3, CYINIT, CIN, DI0, DI1, DI2, DI3, S0, S1, S2, S3);
parameter CYINIT_AX = 1'b0;
parameter CYINIT_C0 = 1'b0; I think this is a poor error message on the part of ABC9, but I think the assert is sound and correct. |
Will |
Yes, though I believe it only removes completely-undriven carry logic. I think the solution is for the Python script to mark all |
In the past, ABC9 was smart enough to completely swap pins and merge apparently redundant logic (from ABC's perspective), so we need something to suppress that behavior. If |
If I'm reading that right, we need the |
Hmm. It seems like I might have to talk to Eddie about this. |
So for clarity the goal is:
|
Here's another approach I'm going to throw out: I know that ABC9 handles boxes marked blackbox fine, the problem is the module also being parametric (which implies potential incompleteness). A hacky idea is to map the cell into a temporary box that contains timing and path information but stores the parameter information via input wires connected to constants, and then map the boxes back to the cell with the right parameters. It's awful, but it works. |
The easiest short term solution would be to remove both |
Do you want me to modify the PR so you can run CI, or are you going to create your own? |
Always appreciate the help. |
Since a workaround got merged, is this going to be closed, or will that wait for the "proper" fix? |
I believe the latest files have the update. @Xiretza are you willing to re-test to see if this is fixed for you? |
I can confirm the issue is now fixed with the removal of the blackbox line. I commented back in the asserts in the two files and they no longer trigger. |
This particular issue seems to be resolved, but I'm hitting the equivalent of #127 in my own project. |
As discussed on IRC, trying to build the xc7/counter_test example results in a failed assertion in yosys during synth with my build configuration. Below are the exact steps to (hopefully) reproduce this issue using vanilla/unpatched yosys, yosys-symbiflow-plugins, symbiflow-arch-defs and prjxray-db.
The failing step is the second ABC9 run, line 158 in
/tmp/reproduce/install/share/symbiflow/scripts/xc7/synth.tcl
.I have previously dug into this crash using gdb and found the erroring cell to be
$auto$alumacc.cc:485:replace_alu$1385.slice[0].carry4
, line 65970 in/tmp/reproduce/symbiflow-examples/xc7/counter_test/build/arty_35/top.json.pre_abc9.ilang
, though that was on my possibly dirty and patched-up system.The text was updated successfully, but these errors were encountered: