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.

Sign up for GitHub

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

Jump to bottom

Rebase master+wip on top master #59

Closed
acomodi opened this issue Mar 5, 2020 · 8 comments
Closed

Rebase master+wip on top master #59

acomodi opened this issue Mar 5, 2020 · 8 comments

Comments

@acomodi
Copy link

acomodi commented Mar 5, 2020

There are issues relative to master+wip rebase on top of master.

There are currently ~1000 commits between master+wip and the master branch. As happens for VPR it would be good to have periodic updates of the master+wip branch to be able to easily spot possible regressions.

Right now, the master+wip update fails the integration with symbiflow-arch-defs. This issue is to keep track of the effort in rebasing the current version to the master branch.
@acomodi
Copy link
Author

acomodi commented Apr 15, 2020 •
edited

I am trying to re-generate the master+wip with the past updates on yosys.

When running symbiflow-arch-defs tests, using the new master+wip, I ran into an ABC issue (for the error_output_logic test in xc/xc7/tests/common:

ABC command line: "source /tmp/yosys-abc-i0BSVF/abc.script".

+ read_lut /usr/local/bin/../share/yosys/xilinx/abc9_xc7.lut
+ read_box /tmp/yosys-abc-i0BSVF/input.box
+ &read /tmp/yosys-abc-i0BSVF/input.xaig
+ &ps
/tmp/yosys-abc-i0BSVF/input : i/o =    151/    144  and =     980  lev =   26 (1.82)  mem = 0.02 MB  box = 33  bb = 13
+ &scorr
Warning: The network is combinational.
+ &sweep
+ &dc2
+ &dch -f
+ &ps
/tmp/yosys-abc-i0BSVF/input : i/o =    151/    144  and =     901  lev =   19 (1.46)  mem = 0.02 MB  ch =  128  box = 33  bb = 13
+ &if -W 300 -v
K = 8. Memory (bytes): Truth =    0. Cut =   64. Obj =  144. Set =  672. CutMin = no
Node =     901.  Ch =   112.  Total mem =    0.23 MB. Peak cut mem =    0.04 MB.
P:  Del = 3843.00.  Ar =    1348.0.  Edge =     1274.  Cut =    14144.  T =     0.00 sec
P:  Del = 3825.00.  Ar =    1385.0.  Edge =     1340.  Cut =    13812.  T =     0.00 sec
P:  Del = 3825.00.  Ar =    1071.0.  Edge =     1107.  Cut =    24151.  T =     0.01 sec
F:  Del = 3816.00.  Ar =     735.0.  Edge =      936.  Cut =    17855.  T =     0.01 sec
A:  Del = 3816.00.  Ar =     666.0.  Edge =      851.  Cut =    16942.  T =     0.01 sec
A:  Del = 3816.00.  Ar =     636.0.  Edge =      821.  Cut =    17224.  T =     0.01 sec
Total time =     0.03 sec
+ &mfs
yosys-abc: src/aig/gia/giaMfs.c:388: Gia_Man_t *Gia_ManInsertMfs(Gia_Man_t *, Sfm_Ntk_t *, int): Assertion `iLitNew >= 0' failed.
Aborted (core dumped)

I tracked down what is causing the issue, and it seems to be related to the addition of the carry4_out box.

By removing the wip/carry4_out branch, the error is solved.

The VPR model though is wrong and CARRY0 is not supported in arch-defs.

@litghost FYI

@acomodi
Copy link
Author

acomodi commented Apr 15, 2020 •
edited

I do not have too much confidence with how ABC works, but I have noticed that, for the abc9_xc7.box, the normal carry4 has 8 outputs, (4 O and 4 CO). This is untrue for the VPR carry4_cout, which has 5 outputs (4 O and 1 COUT).

The problem, IMO, is that in the .box definition, we only use the highest delays, counting as the whole carry is used. This may cause issues in ABC when the carry is not fully used, messing with the math.

I have tried to expand the COUT output to be 4-bits wide, and added the same delays as for the normal carry4:

CARRY4_COUT 8 1 10 8
482     -   -   -   -   223 -   -   -   222
598     407 -   -   -   400 205 -   -   334
584     556 537 -   -   523 558 226 -   239
642     615 596 438 -   582 618 330 227 313
536     379 -   -   -   340 -   -   -   271 # CO0
494     465 445 -   -   433 469 -   -   157 # CO1
592     540 520 356 -   512 548 292 -   228 # CO2
580     526 507 398 385 508 528 378 380 114

This seems to have solved the ABC issue, but I am unsure whether this will cause cascading issues in arch-defs, as now the COUT is 4-bits wide. This change was only done to check where the issue was, it does not mean that it is the correct solution.

@acomodi
Copy link
Author

acomodi commented Apr 15, 2020

The above mentioned addition seems to be working (also on HW) for some basic tests in arch-defs (e.g. counter).

It produces errors though in VPR:

Warning 173: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 2592291 type: CHANX location: (2,132) <-> (9,132) track: 1 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 174: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216436 type: CHANX location: (140,132) <-> (147,132) track: 13 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 175: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216436 type: CHANX location: (140,132) <-> (147,132) track: 13 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 176: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216436 type: CHANX location: (140,132) <-> (147,132) track: 13 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 177: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216436 type: CHANX location: (140,132) <-> (147,132) track: 13 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 178: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216437 type: CHANX location: (140,132) <-> (147,132) track: 12 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 179: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216437 type: CHANX location: (140,132) <-> (147,132) track: 12 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 180: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216439 type: CHANX location: (140,133) <-> (147,133) track: 2 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 181: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216439 type: CHANX location: (140,133) <-> (147,133) track: 2 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 182: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216439 type: CHANX location: (140,133) <-> (147,133) track: 2 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 183: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216439 type: CHANX location: (140,133) <-> (147,133) track: 2 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 184: Inconsitent buffering of children of rr node CCIO_CLK_IN (RR node: 3216440 type: CHANX location: (140,133) <-> (147,133) track: 1 len: 7 longline: 0 seg_type: CCIO_CLK_IN dir: BI_DIR capacity: 1)
Warning 185: Inconsitent bufferin

/data/vtr-symbiflow/vpr/src/place/place_macro.cpp:135 find_all_the_macro: Assertion 'cluster_ctx.clb_nlist.net_sinks(curr_net_id).size() == 1' failed.
xc/xc7/tests/soc/litex/base/CMakeFiles/baselitex_arty_bin.dir/build.make:117: recipe for target 'xc/xc7/tests/soc/litex/base/baselitex_arty/artix7-xc7a50t-virt-xc7a50t-test/top.place' failed

This happens during macro placement and the assertion is relative to some carry_chain nets.

I think that it is caused by the fact that the model/pb_type of the CARRY should be changed accordingly to the change in the techlib

@litghost
Copy link

By removing the wip/carry4_out branch, the error is solved.

Do not remove the carry4-cout branch. It intentionally removed the CO ports because ABC was generating impossible to pack solutions.

@litghost
Copy link

I think that it is caused by the fact that the model/pb_type of the CARRY should be changed accordingly to the change in the techlib

Incorrect. The VPR specific carry chains were correct.

@acomodi
Copy link
Author

acomodi commented Apr 15, 2020

@litghost I wonder whether modifying the abc9_xc7.box and is the right way to go then.

Do not remove the carry4-cout branch

Sure, I did with the only purpose of tracking down the origin of the issue.

Incorrect. The VPR specific carry chains were correct.

Ok, I need to debug this a little bit more to understand what is happening.

@litghost
Copy link

@litghost I wonder whether modifying the abc9_xc7.box and is the right way to go then.

So abc9_xc7.box should not be modified, except to add the VPR specific carry white / black boxes.

@acomodi acomodi mentioned this issue Apr 15, 2020
Closed
@acomodi
Copy link
Author

acomodi commented Dec 8, 2020

This has been fixed. Closing

@acomodi acomodi closed this as completed Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@litghost @acomodi