-
Notifications
You must be signed in to change notification settings - Fork 298
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
[Seq] Add FIFO lowering #5630
[Seq] Add FIFO lowering #5630
Conversation
Lowers to a fairly standard fifo implementation. Will add more target-optimized implementations after this baseline is checked in. For reference, the following: ```mlir hw.module @fifo(%clk : i1, %rst : i1, %inp : i32, %rdEn : i1, %wrEn : i1) -> (out: i32, empty: i1, full: i1, almost_empty : i1, almost_full : i1) { %out, %full, %empty, %almostFull, %almostEmpty = seq.fifo depth 4 almost_full 2 almost_empty 1 in %inp rdEn %rdEn wrEn %wrEn clk %clk rst %rst : i32 hw.output %out, %empty, %full, %almostEmpty, %almostFull : i32, i1, i1, i1, i1 } ``` exports as ```sv module fifo( input clk, rst, input [31:0] inp, input rdEn, wrEn, output [31:0] out, output empty, full, almost_empty, almost_full ); wire [1:0] fifo_rd_addr_next; wire [1:0] fifo_wr_addr_next; wire [2:0] fifo_count_next; reg [1:0] fifo_wr_addr; initial begin $dumpfile ("./fifo.vcd"); $dumpvars (0, fifo); #1; end reg [2:0] fifo_count; always_ff @(posedge clk) begin if (rst) fifo_count <= 3'h0; else fifo_count <= fifo_count_next; end reg [31:0] fifo_mem[0:3]; always_ff @(posedge clk) begin if (rst) begin end else begin if (wrEn) fifo_mem[fifo_wr_addr] <= inp; end end reg [1:0] fifo_rd_addr; always_ff @(posedge clk) begin if (rst) fifo_rd_addr <= 2'h0; else fifo_rd_addr <= fifo_rd_addr_next; end always_ff @(posedge clk) begin if (rst) fifo_wr_addr <= 2'h0; else fifo_wr_addr <= fifo_wr_addr_next; end wire fifo_full = fifo_count == 3'h3; wire fifo_empty = fifo_count == 3'h0; assign fifo_count_next = ~rdEn & ~wrEn ? fifo_count : wrEn & ~rdEn ? (fifo_count == 3'h3 ? fifo_count : fifo_count + 3'h1) : rdEn & ~wrEn ? (fifo_count == 3'h0 ? fifo_count : fifo_count - 3'h1) : fifo_count; assign fifo_wr_addr_next = wrEn & ~fifo_full ? fifo_wr_addr + 2'h1 : fifo_wr_addr; assign fifo_rd_addr_next = rdEn & ~fifo_empty ? fifo_rd_addr + 2'h1 : fifo_rd_addr; assign out = fifo_mem[fifo_rd_addr]; assign empty = fifo_empty; assign full = fifo_full; assign almost_empty = fifo_count <= 3'h1; assign almost_full = fifo_count >= 3'h2; endmodule ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including integration tests!
@@ -33,6 +33,7 @@ std::unique_ptr<mlir::Pass> createLowerSeqHLMemPass(); | |||
std::unique_ptr<mlir::Pass> | |||
createExternalizeClockGatePass(const ExternalizeClockGateOptions &options = {}); | |||
std::unique_ptr<mlir::Pass> createLowerFirMemPass(); | |||
std::unique_ptr<mlir::Pass> createLowerSeqFIFOPass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but at some point we probably want a lowerSeqPass to amortize the walk of the IR over all op types being lowered.
using OpConversionPattern::OpConversionPattern; | ||
|
||
LogicalResult | ||
matchAndRewrite(seq::FIFOOp mem, OpAdaptor adaptor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we've talked about is having a circt.seq verilog package and modules as a stand alone library to eliminate open-coded complex implementations (memories also). Essentially if you can write the parametric module by hand and it's non-trivial, we should probably just be doing that in verilog source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have a generic pass which looks for ops with a special keyword to indicate the name of the module to instantiate which implements a particular op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this - essentially a generic "externalize" pass (which is already being done for e.g. clock gates).
Oops. Posted this comment before reading the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just commenting since I haven’t (yet) checked for correctness.
not necessarily for this pr, but we should probably have to a lowering option to spit out fifo modules which contain all this logic, then the op gets replaced with an instance of said op. Would increase readability and could amortize SV code over many fifo ops with the same attributes.
would this have been easier to implement if we had some sort of counter op in seq?
// ====== Hardware units ====== | ||
Value count = rewriter.create<seq::CompRegOp>( | ||
loc, nextCount, clk, rst, | ||
rewriter.create<hw::ConstantOp>(loc, countType, 0), "fifo_count"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these names also include some fifo instantance name prefix?
Nah, not really. And I'd like to see clear evidence of the ability to provide some interesting optimizations of a counter op before we'd have it.
I was considering this as well, and i fully agree. One issue i have with it, however, is the fact that if we truly want to share/cache these fifo modules as we lower them, we cannot lower on a |
Wouldn't even have to block. Could just have a mutex on the memoization cache wherein threads add something before they start building the module. Other threads would just build the instances which refer to the fifo module symbol -- a completely independent step which doesn't need to have the module built before referring to it. |
Lowers to a fairly standard fifo implementation. Will add more target-optimized implementations after this baseline is checked in.
For reference, the following:
exports as