Implement support for \EN for \TRANSPARENT=1 \CLK_ENABLE=1 $memrd cells

[whitequark]

This case doesn't seem to be described in the manual. It looks like write_verilog completely ignores \EN for anything other than non-transparent synchronous ports. Migen uses "latch address" for this case, which I think matches Xilinx semantics.

What is the intended semantics of this case in Yosys, exactly?

[cliffordwolf]

It looks like write_verilog completely ignores \EN for anything other than non-transparent synchronous ports.

That's consistent with the reference model for $mem in techlibs/common/simlib.v.

However, there's a good argument to be had that it should not be ignored. I think historically I added RD_EN as an afterthought and I only added it to the more common case of non-transparent memories because this was more urgent at the time, and then I forgot about it and never added RD_EN support for transparent read ports.

[whitequark]

OK. So, what should it do? Prevent the output from changing? Would that also apply to asynchronous read ports? It looks like asynchronous write ports are supported (although the Verilog for $mem is kind of confusing and I'm not sure), so it seems like asynchronous read ports should support RE too for parity...

[cliffordwolf]

I'd say for asynchronous read ports EN and TRANSPARENT are without function and should simply be ignored. (There is an argument to be had that in those cases the ignored parameters should have certain "canonical" default values and other values should trigger a runtime error.)

Yes, for "transparent" synchronous read ports the EN signal should probably prevent the output from changing when EN is not active, similar to the non-transparent case.

Asynchronous write ports are just a hacky intermediate representation. They pretty much make only sense if

(a) the address is constant and the memory/array is just a HDL construct for creating many wires, in which case memory_map should be able to replace the memory with something that can be optimized to just a bunch of wires, or

(b) there is a FF that can be merged into the write port, in which case memory_dff will merge that FF into the port and turn it into a synchronous write port.

[whitequark]

Asynchronous write ports are just a hacky intermediate representation. They pretty much make only sense if

Hmm, I assumed that asynchronous read and write ports use EN as a strobe... like good old asynchronous RAM. So effectively there is a latch on the output. Is that not the intent? Then I'll need to document it as such, because right now it's completely ambiguous.

[cliffordwolf]

I assumed that asynchronous read and write ports use EN as a strobe...

For asynchronous write ports it pretty much is, if you ignore the timing horrors that arise from the case where data, address, and enable all change with the same clock event.

I'd say this would actually be a pretty reasonable semantic for async read ports. You could then merge latches into the port, similar to how we merge FFs into the port to make it synchronous. But at least so far this was not really needed.

[whitequark]

Do you think you could make this change? I don't feel very comfortable with the current implementation of $mem cells, and I'm not sure if I can do this without introducing subtle bugs, like with x propagation or something.

[whitequark]

This pattern is what Xilinx recommends:

It's very reasonable. However, it requires knowing which read ports are transparent to which write ports, as suggested by @daveshah1 in https://github.com/YosysHQ/yosys/issues/1390#issuecomment-533696183. So I would say solving this issue requires solving #1390.