circt
circt copied to clipboard
llvm
[SCFToCalyx] Add support for sequential read MemRefType memories
If a FuncOp argument is of MemRefType and has the boolean attribute calyx.sequential_reads set to true, consider the read accesses to that memory as sequential, which adds a "read_en" signal to the external memory interface. This read_en signal is driven by a memref::LoadOp to a memref with sequential reads.
A register that stores the data read from memory and forwarded to the next group is created.
Currently, the memory access ports contain a single "done" signal, therefore it's not possible to perform a memory write and load at the same time.
This allows interfacing (with extra logic) with protocols such as AXI4.
@mortbopet Thanks for the feedback! I've applied the changes you suggested.
Also thinking that maybe bool MemoryInterface::sequentialReads() could be removed and instead check for Value MemoryInterface::readEn() to be non-null.
Another problem is that if the memref has no loads/stores, the corresponding write_en/read_en signal will never be driven, and therefore the pass to SV will assign z.
Should we check if there are no loads/stores to the memref, and if so force the corresponding *_en signal to 0?
Another problem is that if the memref has no loads/stores, the corresponding write_en/read_en signal will never be driven, and therefore the pass to SV will assign z. Should we check if there are no loads/stores to the memref, and if so force the corresponding *_en signal to 0?
Hmm... In the spirit of keeping this pass atomic, would it make more sense to have a later pass which legalizes the Calyx IR? That is, there may be other passes and optimizations which end up removing output signal drivers, and so these will also require some legalization to drive those outputs to either 0, x or z.
Another problem is that if the memref has no loads/stores, the corresponding write_en/read_en signal will never be driven, and therefore the pass to SV will assign z. Should we check if there are no loads/stores to the memref, and if so force the corresponding *_en signal to 0?
Hmm... In the spirit of keeping this pass atomic, would it make more sense to have a later pass which legalizes the Calyx IR? That is, there may be other passes and optimizations which end up removing output signal drivers, and so these will also require some legalization to drive those outputs to either
0,xorz.
Indeed, sounds good to me!
Some extra relevant information: Calyx does have memories that support sequential reads and writes and we have a plan to deprecate the current std_mem_* and replace them with seq_mem_* equivalents (https://github.com/cucapra/calyx/issues/1261). The seq_mem_* are defined in this verilog file and have the following Calyx interfaces. I think @andrewb1999 and @matth2k might have some work on reimplementing SCFToCalyx to use those kinds of memories.
@xerpi are you still trying to get this merged? If so, can you fix the merge conflicts?
@xerpi are you still trying to get this merged? If so, can you fix the merge conflicts?
I see a couple of differences between what this MR implements and Calyx memory primitives (if I'm not mistaken):
- All Calyx memory primitives have "sequential reads" by default (
read_en) - Calyx memories read and write ports have individual
donesignals:read_enandwrite_en
Should the implementation be changed to align it with the Calyx standard for consistency?
Yeah, I think that sounds like a reasonable plan! @andrewb1999 has proposed a change to the seq_mem implemented in Calyx to use a content_en signal in the future (https://github.com/cucapra/calyx/pull/1610) but maybe this PR can serve as the scaffolding for that future change.
I believe this can be closed now, since https://github.com/llvm/circt/pull/5471 added support for sequential memories, correct?
Yup! @jiahanxie353 that sound about right?
Yes, I agree!
Feel free to close it @xerpi :)
