Implement support for \EN for \TRANSPARENT=1 \CLK_ENABLE=1 $memrd cells #760
Comments
That's consistent with the reference model for However, there's a good argument to be had that it should not be ignored. I think historically I added |
|
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 |
|
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 (b) there is a FF that can be merged into the write port, in which case |
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. |
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. |
|
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. |
This commit fixes two related issues:
* For asynchronous ports, clock is no longer added to domain list.
(This would lead to absurd constructs like `always @(posedge 0)`.
* The logic to distinguish synchronous and asynchronous ports is
changed to correctly use or avoid clock in all cases.
Before this commit, the following RTLIL snippet (after memory_collect)
cell $memrd $2
parameter \MEMID "\\mem"
parameter \ABITS 2
parameter \WIDTH 4
parameter \CLK_ENABLE 0
parameter \CLK_POLARITY 1
parameter \TRANSPARENT 1
connect \CLK 1'0
connect \EN 1'1
connect \ADDR \mem_r_addr
connect \DATA \mem_r_data
end
would lead to invalid Verilog:
reg [1:0] _0_;
always @(posedge 1'h0) begin
_0_ <= mem_r_addr;
end
assign mem_r_data = mem[_0_];
Note that there are two potential pitfalls remaining after this
change:
* For asynchronous ports, the \EN input and \TRANSPARENT parameter
are silently ignored. (Per discussion in YosysHQ#760 this is the correct
behavior.)
* For synchronous transparent ports, the \EN input is ignored. This
matches the behavior of the $mem simulation cell. Again, see YosysHQ#760.
This is necessary for consistency, since for transparent read ports, we currently do not support .en at all (it is fixed at 1) due to YosysHQ/yosys#760. Before this commit, changing port transparency would require adding or removing an assignment to .en, which is confusing and error-prone. Also, most read ports are always enabled, so this behavior is also convenient.
whitequark
changed the title
|
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 #1390 (comment). So I would say solving this issue requires solving #1390. |
This case doesn't seem to be described in the manual. It looks like
write_verilogcompletely ignores\ENfor 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?
The text was updated successfully, but these errors were encountered: