abc commit 0d0063f7 breaks tests/arch/ice40/rom.ys

[DanielG]

Hi,

in Debian we package berkeley-abc master to use with yosys (using the 0.30 release). I've started seeing test failures a while back and only now found time to investigate. After cherry-picking a fix for the ioWriteVerilog segfault (commit) I'm seeing a test failure in tests/arch/ice40/rom.ys that also happened with the latest YosysHQ/abc. I ran a git-bisect with yosys-abc and found that the following commit is to blame (which is also present in berkeley-abc master):

commit 0d0063f7de69388c80e4a6adc4bfc27287a3e19a
Author: Alan Mishchenko <[email protected]>
Date:   Tue Feb 28 13:50:35 2023 +0700

    Experiment with cost functions.

git bisect log:

# bad: [bb64142b07794ee685494564471e67365a093710] Merge pull request #23 from jix/fold-s
# good: [2c1c83f75b8078ced51f92c697da3e712feb3ac3] Merge pull request #21 from xobs/cast-unsigned-signed
git bisect start 'yosys-experimental' 'HEAD'
# bad: [eaa9da53cd89673af3d9c2df7f6f15ed11b5b172] Various unrelated changes.
git bisect bad eaa9da53cd89673af3d9c2df7f6f15ed11b5b172
# good: [bbd0640db2b0709c06022341690b2b4dc8256612] Enable 'scorr' when AIG has no internal nodes.
git bisect good bbd0640db2b0709c06022341690b2b4dc8256612
# bad: [9d0e828b85e6f47f0d8af64a50a0b8c1f0499342] Fixing compiler error.
git bisect bad 9d0e828b85e6f47f0d8af64a50a0b8c1f0499342
# bad: [622d142794e70a16b1f7ec8c5f0a476743d2801a] Compiler warnings.
git bisect bad 622d142794e70a16b1f7ec8c5f0a476743d2801a
# good: [581c58b9c48772b549dc921fd7c854484470ed8c] Experiment with choice computation.
git bisect good 581c58b9c48772b549dc921fd7c854484470ed8c
# bad: [b57b54649437636e2734233417272dc6f24e6964] Compiler warnings.
git bisect bad b57b54649437636e2734233417272dc6f24e6964
# bad: [0d0063f7de69388c80e4a6adc4bfc27287a3e19a] Experiment with cost functions.
git bisect bad 0d0063f7de69388c80e4a6adc4bfc27287a3e19a
# first bad commit: [0d0063f7de69388c80e4a6adc4bfc27287a3e19a] Experiment with cost functions.

The test failure looks like this:

ABC=$HOME/dev/fpga/yosys-abc/abc "/home/dxld/share/dev/deb/pkg/yosys/yosys" -ql rom.log -w 'Yosys has only limited support for tri-state logic at the moment.' rom.ys
Warning: wire '\data' is assigned in a block at rom.v:10.5-10.15.
Warning: wire '\data' is assigned in a block at rom.v:11.5-11.15.
Warning: wire '\data' is assigned in a block at rom.v:12.5-12.15.
Warning: wire '\data' is assigned in a block at rom.v:13.6-13.16.
Warning: wire '\data' is assigned in a block at rom.v:14.6-14.16.
Warning: wire '\data' is assigned in a block at rom.v:15.6-15.16.
Warning: wire '\data' is assigned in a block at rom.v:16.11-16.21.
ERROR: Assertion failed: selection contains 6 elements instead of the asserted 5: t:SB_LUT4
Selection contains:
top/data_SB_LUT4_O_2_I1_SB_LUT4_O
top/data_SB_LUT4_O_1_I3_SB_LUT4_O
top/data_SB_LUT4_O
top/data_SB_LUT4_O_I2_SB_LUT4_O
top/data_SB_LUT4_O_1
top/data_SB_LUT4_O_2

I would like to package the yosys-abc fork for Debian but since no releases are tagged for it our tooling doesn't allow this easily. cf. YosysHQ/prjtrellis#226.

Thanks,
--Daniel

[Ravenslofty]

Hmm, I don't think this is caused by that commit. The YosysHQ/abc fork includes that commit, so #3825 would have failed CI if the ABC update brought changes that affect synthesis.

My suspicion is actually on a compiler change or something; ABC does some very weird things that might cause a compiler to order things differently. Given that the CI uses the same compilers and distros, this seems the most likely variable to me?

[DanielG]

Possible. However with the environment I'm testing in, reverting the mentioned commit does fix the test failure so it's at least partially at fault.

Here's a dump of package versions used in the build: https://dxld.at/localdebs/sid/berkeley-abc_1.01+20230625git01b1bd1+dfsg-3_amd64.buildinfo Most notably we're building with gcc-12 (g++ (= 4:12.3.0-1), gcc (= 4:12.3.0-1). Though FYI migration to gcc-13 is imminent in Debian unstable.

[jix]

The relevant ABC revision bump was #3796 not #3825, and there the tests got updated as they were failing. IIRC, those tests matched for a specific output that could plausibly change when abc's heuristics change and there wasn't anything actually broken.

@mmicko: can we also tag our abc fork with the corresponding yosys release, like we started doing with SBY?

[mmicko]

@jix yes, we can do that. Also it is possible to do for past releases if needed, since we know exact version used

[mmicko]

@DanielG Now the tags are updated for yosys abc (https://github.com/YosysHQ/abc/tags) and they are named same as tags in yosys itself, so it will be maintained in future (same as for sby). Also note that tags are added for past releases as well.

[DanielG]

@mmicko excellent. Particularly having the matching tags even if the abc commit doesn't change is appreciated :)

One nit: the current tag format is suboptimal, you end up with abc tarballs being called something like yosys-0.32.tar.gz, it would be better to leave off the yosys- prefix or rename it to yosys-abc-. I can fix this in the debian/watch rules either way but other tooling might not be so forgiving there.