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

Jump to bottom

Memory port transparency model is flawed #216

Closed
nmigen-issue-migration opened this issue Sep 20, 2019 · 9 comments · Fixed by #1142
Closed

Memory port transparency model is flawed #216

nmigen-issue-migration opened this issue Sep 20, 2019 · 9 comments · Fixed by #1142
Labels
bug

Comments

@nmigen-issue-migration
Copy link

Issue by whitequark
Friday Sep 20, 2019 at 18:04 GMT
Originally opened as m-labs/nmigen#216

Investigating #172, it appears that there are two major issues with the way port transparency is handled.

  1. Transparent read ports should not be the default, since it is expensive. On many platforms (e.g. iCE40), fabric logic will need to be inserted. On other platforms (e.g. Xilinx), non-transparent ports have a much larger set of configurations with well-defined behavior. Transparency between different clock domains is impossible with BRAMs and will result in a (silent) failure during synthesis.
  2. Transparency should not be requested in general, but rather for a specific write port (in the same domain). It does not make sense to request a "fully" transparent read port for a true dual-port RAM with write ports in different domains. It does make sense to request a read port transparent only wrt one specific write port for a true dual-port RAM with write ports in the same domain (this is implementable in Xilinx) and a read port transparent wrt both write ports for a true dual-port RAM with write ports in the same domain (this is not directly implementable in any FPGA arch I know, but is useful for e.g. superscalar CPU register files).

I propose to remove transparent completely, replacing it with a transparent_for=[write_ports] argument. The default (when this argument is not specified) would be "transparent for no write ports" for synchronous read ports, and "transparent for all ports" for asynchronous read ports. It would not be possible to specify a non-default value for an asynchronous read port.

The lowering to RTLIL would be the same as today, with the selected write port ignored (beyond checking that the domain is compatible),
@nmigen-issue-migration
Copy link
Author

Comment by whitequark
Friday Sep 20, 2019 at 18:21 GMT

According to Clifford, the TRANSPARENT parameter of $memrd only applies to the ports in the same domain, so point (2) does not necessarily apply. (It would be still be necessary to clearly document the semantics of transparent read ports.)

@nmigen-issue-migration
Copy link
Author

Comment by whitequark
Friday Sep 20, 2019 at 18:23 GMT

According to UG473, WRITE_FIRST (i.e. transparent) ports are recommended for TDP, and READ_FIRST ports would, confusingly, result in an undefined value being read. This doesn't seem to make any sense to me, so I'm looking further.

@nmigen-issue-migration
Copy link
Author

Comment by whitequark
Friday Sep 20, 2019 at 18:51 GMT

According to @nakengelhardt, a read/write collision between asynchronous ports always results in undefined data being read. Apparently, using the WRITE_FIRST is recommended because it results in lower power consumption.

Based on this and m-labs/nmigen#216 (comment), the current semantics actually seems fine (if confusing), and so #172 is likely caused by something else.

@nmigen-issue-migration
Copy link
Author

Comment by whitequark
Friday Sep 20, 2019 at 20:33 GMT

Reopening per discussion in YosysHQ/yosys#1390.

@nmigen-issue-migration
Copy link
Author

Comment by whitequark
Monday Sep 23, 2019 at 15:05 GMT

After careful consideration, it is my opinion that transparent_for should be used as soon as possible, even if it means desugaring to \TRANSPARENT=1 in Yosys at first. Including in 0.1.

@nmigen-issue-migration
Copy link
Author

Comment by whitequark
Friday Oct 04, 2019 at 07:35 GMT

After discussion with Yosys maintainers, the most likely way this will be solved upstream is by (a) keeping \TRANSPARENT working as it currently is, (b) splitting ports into groups (which are most of the time identical to clock domains), (c) making \TRANSPARENT and \PRIORITY act per-group, not per-memory.

This is a less invasive solution than the transparent_for one I have suggested above (and is therefore a better one). Moreover, we do not need a radical change in the API, and there is no need to change anything for 0.1; we can add a new API and a warning at any point in the future without breaking existing code. Denominating from 0.1.

@galibert
Copy link
Contributor

galibert commented Mar 5, 2020

If there a way to say "I'm not going to read and write in the same cycle ever, so do what's best given the platform"?

@whitequark
Copy link
Member

No. Yosys doesn't support it.

@daveshah1 daveshah1 mentioned this issue Apr 17, 2020
Closed
12 tasks
@whitequark
Copy link
Member

Yosys now supports a much better memory port transparency model.

This issue will be solved by amaranth-lang/rfcs#45.

@wanda-phi wanda-phi mentioned this issue Feb 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement RFC 45: Move hdl.Memory to lib.Memory. wanda-phi/amaranth
3 participants
@whitequark @galibert @nmigen-issue-migration