[LowerToHW] Error in firtool-1.54.0 Memory Lowering w/ Clock Type
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
I ran into a bug with `firtool`'s lowering of FIRRTL memories. If you have a memory that includes a `Clock` type, then `LowerToHW` creates a wire of type `(i1) -> !seq.clock` which the verifier is unhappy about.
Example:
```
circuit Foo:
module Foo:
input r: {addr: UInt<3>, en: UInt<1> clk: Clock, flip data: {a: Clock}}
input w: {addr: UInt<3>, en: UInt<1> clk: Clock, data: {a: Clock}, mask: {a: UInt<1>}}
mem memory:
data-type => {a: Clock}
depth => 16
reader => r
writer => w
read-latency => 1
write-latency => 1
read-under-write => undefined
memory.r <= r
memory.w <= w
```
Compiling with `firtool-1.54.0 Memory.fir`:
```
Memory.fir:6:5: error: 'hw.wire' op requires the same type for all operands and results
mem memory:
^
Memory.fir:6:5: note: see current operation: %2 = "hw.wire"(%5) {name = "memory_r_data_a"} : (i1) -> !seq.clock
```
The failing MLIR is:
```mlir
#loc = loc("Memory.fir":3:11)
#loc1 = loc("Memory.fir":4:11)
"builtin.module"() ({
"hw.module"() ({
^bb0(%arg0: i3, %arg1: i1, %arg2: !seq.clock, %arg3: i3, %arg4: i1, %arg5: !seq.clock, %arg6: !seq.clock, %arg7: i1):
%0 = "hw.constant"() {value = false} : () -> i1
%1 = "hw.wire"(%7) {name = "memory_r_addr"} : (i4) -> i4
%2 = "hw.wire"(%5) {name = "memory_r_data_a"} : (i1) -> !seq.clock
%3 = "hw.wire"(%8) {name = "memory_w_addr"} : (i4) -> i4
%4 = "seq.firmem"() <{name = "memory_a", prefix = "", readLatency = 1 : i32, ruw = 0 : i32, writeLatency = 1 : i32, wuw = 1 : i32}> : () -> !seq.firmem<16 x 1>
%5 = "seq.firmem.read_port"(%4, %1, %arg2, %arg1) : (!seq.firmem<16 x 1>, i4, !seq.clock, i1) -> i1
%6 = "comb.and"(%arg4, %arg7) <{twoState}> : (i1, i1) -> i1
"seq.firmem.write_port"(%4, %3, %arg5, %6, %arg6) <{operandSegmentSizes = array}> : (!seq.firmem<16 x 1>, i4, !seq.clock, i1, !seq.clock) -> ()
%7 = "comb.concat"(%0, %arg0) : (i1, i3) -> i4
%8 = "comb.concat"(%0, %arg3) : (i1, i3) -> i4
"hw.output"(%2) : (!seq.clock) -> ()
}) {argLocs = [#loc, #loc, #loc, #loc1, #loc1, #loc1, #loc1, #loc1], argNames = ["r_addr", "r_en", "r_clk", "w_addr", "w_en", "w_clk", "w_data_a", "w_mask_a"], arg_attrs = [{}, {}, {}, {}, {}, {}, {}, {}], comment = "", function_type = (i3, i1, !seq.clock, i3, i1, !seq.clock, !seq.clock, i1) -> !seq.clock, parameters = [], res_attrs = [{}], resultLocs = [#loc], resultNames = ["r_data_a"], sym_name = "Foo"} : () -> ()
}) : () -> ()
```
If I compile with `firtool-1.53.0 Memory.fir -disable-all-randomization`, I get the following Verilog:
```verilog
// Generated by CIRCT firtool-1.53.0
// VCS coverage exclude_file
module memory_a_16x1(
input [3:0] R0_addr,
input R0_en,
R0_clk,
input [3:0] W0_addr,
input W0_en,
W0_clk,
W0_data,
output R0_data
);
reg Memory[0:15];
reg _R0_en_d0;
reg [3:0] _R0_addr_d0;
always @(posedge R0_clk) begin
_R0_en_d0 <= R0_en;
_R0_addr_d0 <= R0_addr;
end // always @(posedge)
always @(posedge W0_clk) begin
if (W0_en)
Memory[W0_addr] <= W0_data;
end // always @(posedge)
assign R0_data = _R0_en_d0 ? Memory[_R0_addr_d0] : 1'bx;
endmodule
module Foo(
input [2:0] r_addr,
input r_en,
r_clk,
input [2:0] w_addr,
input w_en,
w_clk,
w_data_a,
w_mask_a,
output r_data_a
);
memory_a_16x1 memory_a_ext (
.R0_addr ({1'h0, r_addr}),
.R0_en (r_en),
.R0_clk (r_clk),
.W0_addr ({1'h0, w_addr}),
.W0_en (w_en & w_mask_a),
.W0_clk (w_clk),
.W0_data (w_data_a),
.R0_data (r_data_a)
);
endmodule
```
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the LowerToHW pass and the firtool path that lowers the Memory.fir reproduction, then inspect the failing MLIR around the hw.wire for memory_r_data_a. Compare the result with firtool-1.53.0; done means Clock-typed memory lowering verifies successfully and produces valid output for the supplied example.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100