llvm / llvm/circt

[LowerToHW] Error in firtool-1.54.0 Memory Lowering w/ Clock Type

Open
#6,272 0 comments 0 reactions 0 assignees View on GitHub
bug FIRRTL HW
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.