Issue with memory inference on variables with async reset

[alikates]

After a couple really small changes to the RTL I managed to have the Sargantana core totally parsed. But when trying to synthesize for FPGA (ECP5 in this case) I'm facing this error, which is related to the module rtl/datapath/rtl/exe_stage/rtl/div_unit.sv:

2.26. Executing MEMORY_LIBMAP pass (mapping memories to cells).
found attribute 'ram_style = logic' on memory div_unit.cycles_counter, forced mapping to FF
Rejected candidates for mapping memory div_unit.cycles_counter:
can't map to logic: ports have different write clock domains
ERROR: no valid mapping found for memory div_unit.cycles_counter

I took a look at the RTL and this seems to be caused by a pattern equivalent to this:

module multiport_mem #(
    parameter integer NUM_COUNTERS = 2,
    localparam integer NUM_COUNTERS_BITS = NUM_COUNTERS == 1 ? 0 : $clog2(NUM_COUNTERS)-1
) (
    input logic clk_i,
    input logic rstn_i,
    input logic [NUM_COUNTERS_BITS:0] sel_i,
    input logic sync_rst_i,
    output logic [5:0] count
);
    logic [5:0] counter [NUM_COUNTERS-1:0];

    assign count =  counter [sel_i];

    always_ff @( posedge clk_i or negedge rstn_i ) begin : blockName
        if (~rstn_i) begin
            for (integer i =0; i < NUM_COUNTERS; i++) begin
                counter[i] <= 6'd0;
            end
        end else if (sync_rst_i) begin
            counter[sel_i] <= 6'd0;
        end else begin
            for (integer i =0; i < NUM_COUNTERS; i++) begin
                if (counter[i] != 6'd0) begin
                    counter[i]   <= counter[i] - 6'd1;
                end
            end
        end
    end
endmodule

As you can see, the counter memory is not totally reset, but only the part selected by sel_i. I checked the RTLIL and found out that for the async reset, the clock of the memory cell is the rstn_i signal, and the regular write port uses clk_i as expected. On the other hand, I'm seeing that for the read side, the synchronous and asynchronous reset inputs are always tied to 0. I assume for correctly setting the resets we would need to detect which signal is a reset (i guess the most simple heuristic would be if it is inside the sensitivity list and also enables writes to the memory).

[alikates]

I noticed this is independent of the sync reset, it seems to happen whenever there's an async reset. Let me check if theres any memory with async reset that is being correctly inferred...

[alikates]

Oops, I just noticed that 1) async reset detection is already implemented as proc_arst at RTLIL level, and 2) that pass was not being called by the frontend. Nvm I just saw bc6b5eec509007d2ce12f4ab7c13ef6fbc4fa4b9, so I guess we have to do it anyways

[alikates]

So, going back to what I thought about previously. Add the current UpdateTiming to SignalEvalContext and infer the reset by checking if the signal is in the sensitivity and also drives the memory write port. The only thing I found a bit annoying but probably has a reason is that we have to infer the async-reset signal from the $memwr cell, but use it as input for $memrd

[povik]

~Oops, I just noticed that 1) async reset detection is already implemented as proc_arst at RTLIL level, and 2) that pass was not being called by the frontend.~ Nvm I just saw bc6b5ee, so I guess we have to do it anyways

Yes, to give context on the choice not to rely on sync rules: YosysHQ/yosys#4231 amaranth-lang/amaranth#603

[povik]

The only thing I found a bit annoying but probably has a reason is that we have to infer the async-reset signal from the $memwr cell, but use it as input for $memrd

I don't think this transformation is functionally correct. The ARST input on $memrd_v2 does not reset the memory content, only the current output of the read port, if the read port is a synchronous one.

The way out here is to make sure variables are not inferred as memories if they are written to from the asynchronous branch of the async always_ff pattern.

[alikates]

Okay, sorry. I didn't know about the implications of memory cells. Then if its async reset, disqualify for inference and go for plain DFFs right?

[povik]

Yes, exactly!

[povik]

Fixed in 5aaa6e7