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

Yosys assertion when converting SyncFIFO #617

Closed
anuejn opened this issue May 30, 2021 · 7 comments
Closed

Yosys assertion when converting SyncFIFO #617

anuejn opened this issue May 30, 2021 · 7 comments

Comments

@anuejn
Copy link
Contributor

anuejn commented May 30, 2021

SyncFIFO cant be used with recent yosys anymore.

Probably related to YosysHQ/yosys@c707649 ?

repro:

from nmigen import *
from nmigen.back import verilog
from nmigen.lib.fifo import SyncFIFO
verilog.convert(SyncFIFO(width=8, depth=8))

results in:

nmigen._toolchain.yosys.YosysError: ERROR: Assert `!port.transparent' failed in kernel/mem.cc:293.
@whitequark
Copy link
Member

Please report this against Yosys,

@anuejn
Copy link
Contributor Author

anuejn commented Jun 1, 2021

from the yosys issue:

By the way, there is no such thing as a "transparent" asynchronous read port, and it never behaved differently from a plain one. It just so happens that a check function meant to trigger on invalid memory configs was added, and caught this.

It seems like we should reflect this in the nMigen Memory / FIFO?

@mwkmwkmwk
Copy link
Contributor

Fixed on the yosys side, sorry for the mess (this issue will only affect yosys master compiled within the last 9 days). $memrd cells without a clock are now once again accepted regardless of the (effectively meaningless) value of the TRANSPARENT parameter. $memrd_v2 will follow more strict rules when it lands, but then it's very likely to have per-port-pair transparency masks instead.

@whitequark
Copy link
Member

We should also set TRANSPARENT to whatever Yosys expects for such ports. That code was written a very long time ago and IIRC it predates any documentation on $memrd cells, which I wrote using the code as reference. So it's not unexpected that the nMigen side of this is kinda wrong.

@mwkmwkmwk
Copy link
Contributor

Verilog frontend uses 0, hence my expectation in the check. However, I don't see a point of changing this — I explicitely don't want to break compatibility with old RTLIL inputs and $memrd should now be accepted according to the old rules. And when we migrate to $memrd_v2 for the the new features, we'll get to deal with transparency masks instead.

@whitequark
Copy link
Member

Alright.

@anuejn
Copy link
Contributor Author

anuejn commented Jun 8, 2021

closing this as it is fixed upstream :)

@anuejn anuejn closed this as completed Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants